Inkscape: A Vector Drawing Tool

mod->set_param_string memory leak

Reported by David Mathog on 2012-08-29
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Medium
Unassigned

Bug Description

Valgrind shows a small memory leak associated with calling mod->set_param_string(). This function is actually called twice in
emf_print_document_to_file() which was in turn called by Emf::save() and each of these generated a valgrind message. One is shown below. These are the only two mod->set_param_*() in this code, but they are called on every file save.

==21262== 16 bytes in 1 blocks are definitely lost in loss record 7,095 of 20,681
==21262== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==21262== by 0x8502DBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==21262== by 0x8502EAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==21262== by 0x8343420: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:376)
==21262== by 0x80DCD82: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==21262== by 0x81F74A9: Inkscape::Extension::ParamString::set(char const*, SPDocument*, Inkscape::XML::Node*) (string.cpp:64)
==21262== by 0x824EB76: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:283)
==21262== by 0x81FB006: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==21262== by 0x81F99CF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==21262== by 0x80AF67E: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp:615)
==21262== by 0x80AF0CE: sp_file_save_dialog(Gtk::Window&, SPDocument*, Inkscape::Extension::FileSaveMethod) (file.cpp:886)
==21262== by 0xBD2C48B: ???

The call(s) that triggered this were in my EMF code, in lines copied verbatim from this file:

latex-pstricks-out.cpp: mod->set_param_string("destination", filename);
latex-pstricks-out.cpp: mod->set_param_string("destination", oldoutput);

It looks like there should be some sort of release_param_string() call to free the memory. Is there a method like that?

Kris (kris-degussem) on 2012-09-02
tags: added: performance
Kris (kris-degussem) wrote :

With revision 11646, some memory leaks should have been fixed. Could you have a look whether it solves this issue?

Kris (kris-degussem) wrote :

rev 11646 caused some issues. Reverted temporarily

Kris (kris-degussem) wrote :

The patch in attachment should fix four memory leaks related to not releasing memory occupied by sp_repr_css_write_string.
Hopefully this patch does not lead to errors such as in the reverted revision.

Can someone check with valgrind whether it solves the above bug? I can not check and I'm not sure ... If it does not solve the issue, then quite probably the culprit is that the memory occupied by share_string on line 375 of xml/simple-node.cpp (new_value = share_string(cleaned_value);) is not released.

Kris (kris-degussem) wrote :

For completeness the issues referred to in comment 2 are mentioned in bug #1045117 though they should not be present anymore with the patch in comment 3.

jazzynico (jazzynico) on 2012-09-10
Changed in inkscape:
importance: Undecided → Medium
status: New → In Progress
Kris (kris-degussem) wrote :

Minor update to patch (some more string class usage) and up to date with trunk r11675.
If no side effects detected in the next two days, I will commit the patch.

~suv (suv-lp) wrote :

> If no side effects detected in the next two days, I will commit the patch.

None of the issues described in bug #1045117 reproduced with last version of patch on OS X 10.7.4 (dbus builds without failure, no objects get autolocked when imported or newly created, locked objects and layers can be unlocked again). Haven't done other tests beyond this (tested on OS X 10.7.4, 64bit).

Kris (kris-degussem) wrote :

Patch committed in trunk r11686.
Bug status still to be checked.
David: could you have a look with valgrind if the issue is solved?

David Mathog (mathog) wrote :
Download full text (4.9 KiB)

I have 11679 as branch lp988601, is the problem patch in or out of that version? setAttribute() is still causing problems, so I am guessing no. But I could not apply the patch from (5) successfully:

 patch -p0 </tmp/1043571v2.patch
patching file src/attribute-rel-util.cpp
Reversed (or previously applied) patch detected! Assume -R? [n]
Apply anyway? [n] y
Hunk #1 FAILED at 128.
Hunk #2 FAILED at 147.
Hunk #3 FAILED at 155.
3 out of 3 hunks FAILED -- saving rejects to file src/attribute-rel-util.cpp.rej
patching file src/attribute-rel-util.h
Reversed (or previously applied) patch detected! Assume -R? [n]
etc.

In any case, 11679/lp988601 has other issues which do not fall on setAttribute lines, and there does not seem to be anything in the repr-css.cpp part of 1043571v2.patch which addresses them (possibly addressed elsewhere in the patch though):

1. just after the comment containing

    * HACK for now is to strip off em and ex units and add them back at the end

there is a piece of code that does not check that l>2 before using strncmp, which spammed the valgrind output
with 40 something invalid reads. It should be:

    int l = strlen(value_unquoted);
    if (l>2 &&
            (!strncmp(&value_unquoted[l-2], "em", 2) ||
             !strncmp(&value_unquoted[l-2], "ex", 2)
            )) {
        units = g_strndup(&value_unquoted[l-2], 2);
        value_unquoted = g_strndup(value_unquoted, l-2);
    }

Also, please have mercy and don't use "l" for a variable, it is nearly indistinguishable from "1", especially if your eyes are as
bad as mine.

2. (many) leak(s) at line 343
    dst->mergeFrom(src, "");

here is one

==5076== 160 (128 direct, 32 indirect) bytes in 8 blocks are definitely lost in loss record 51,144 of 58,146
==5076== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x834A9B2: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (gc-core.h:74)
==5076== by 0x8347CAD: Inkscape::XML::SimpleNode::mergeFrom(Inkscape::XML::Node const*, char const*) (simple-node.cpp:660)
==5076== by 0x833D814: sp_repr_css_merge(SPCSSAttr*, SPCSSAttr*) (repr-css.cpp:343)
==5076== by 0x8B08327: ???

3. 1 leak at 351 (6200 bytes in the test run)

    guchar *const str_value_unsigned = cr_term_to_string(decl->value);

when called from 410

    sp_repr_css_merge_from_decl(css, decl_list);

==5076== 6,200 bytes in 25 blocks are possibly lost in loss record 57,124 of 58,146
==5076== at 0x402A420: memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x402A4DE: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5076== by 0x52FF2E1: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534B73F: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534EEE9: g_string_sized_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x534F526: g_string_new (in /lib/i386-linux-gnu/libglib-2.0.so.0.3200.3)
==5076== by 0x86CE81E: cr_term_to_string (cr-term.c:287)
==5076== by 0x833DD09: _ZL27sp_repr_css_merge_from_declP9SPCSSAttrPK14_CRDeclaration.isra.27 (repr-css.cpp:35...

Read more...

David Mathog (mathog) wrote :

How is "bzr merge" used? I copied the two files I modified to a safe place and then did:

bzr revert #because there was no "commit"
bzr merge #to bring the branch in sync with trunk
bzr: ERROR: Cannot lock LockDir(http://bazaar.launchpad.net/~inkscape.dev/inkscape/lp988601/.bzr/branch/lock): Transport operation not possible: http does not support mkdir()

Doesn't merge take the changes from trunk and merge them into the local branch? Why would it need a lock file?

Kris (kris-degussem) wrote :

I have difficulties understanding what you mean (comment 8).
If I'm right, you tested with trunk revision 11679, which does not include the modifications of the patch (committed in revision 11686).

1. is a new issue. I will have a look at it. There is a memory leak at line 367 ("value_unquoted = g_strndup(value_unquoted, l-2);"). Will see whether using the Glib::ustring class as alternative to C-strings can be used.

2. is the original issue of this report. I'm not sure it is solved with revision 11686 (I actually doubt). If not solved, it should be checked whether the share_string function (used on line 375 of xml/simple-node.cpp) could be converted to use c++ string classes as opposed to C-strings/shared pointer strings.

Kris (kris-degussem) wrote :

(some more on comment 8)
3. should be fixed with attached patch (not including any other changes previously reported as they are already in trunk). If I'm right though the library in which the code is, is deprecated though.

Changed in inkscape:
assignee: nobody → Kris (kris-degussem)
Kris (kris-degussem) wrote :

(comment 9): What do you want to do? Do you need:
   bzr pull

Do you want to override local changes and download modifications from trunk? (if so "bzr pull --overwrite" should do the trick)

David Mathog (mathog) wrote :

(comment 12). lp988601 is based on revision 11679. There are changes incorporated in lp988601 that are post r11689, some
are in the branch that I downloaded, and some were made locally after that. There are also changes in trunk since r11679. I want to modify lp988601 so that the local copy has both sets of changes applied.

What I had been doing before ~suv made the lp988601 branch was to keep all of my changes in a big patch file. To start
work on a new trunk revision, it would be downloaded with

  bzr checkout --lightweight lp:inkscape

and then the patches would be applied to it with

  patch -p0 <patches"

For some reason the patch command doesn't work with lp988601 though, even when I know the patch isn't in lp988601, it still gives messages like at the beginning of (8).

Kris (kris-degussem) wrote :

(comment 13) OK: r11678 from the branch https://code.launchpad.net/~inkscape.dev/inkscape/lp988601 includes the revisions in trunk up to revision r11698, so it is normal what you get (you can not add a modification in the code that is already included :-) ). So my question in comment 10 is solved then and some more work (on the long term) is needed.

~suv (suv-lp) wrote :

> I have difficulties understanding what you mean (comment 8).
> If I'm right, you tested with trunk revision 11679, which does not include
> the modifications of the patch (committed in revision 11686).

'lp988601' (comment #8) refers to the branch I created for the enhanced EMF support (tracked in bug #988601):
<https://code.launchpad.net/~inkscape.dev/inkscape/lp988601>

(I have now updated the branch to include the latest changes from trunk up to r11704).

David Mathog (mathog) wrote :
Download full text (6.9 KiB)

Used "bzr update" to bring my local copy up to the changes ~suv noted in #15. Tested the updated lp988601 and it is still broken.

This is emf-inout.cpp:308

    mod->set_param_string("destination", oldoutput);

and here is the associated valgrind record:

==3499== 22 bytes in 1 blocks are possibly lost in loss record 30,463 of 58,207
==3499== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499== by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499== by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499== by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499== by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499== by 0x8200199: Inkscape::Extension::ParamString::set(char const*, SPDocument*, Inkscape::XML::Node*) (string.cpp:64)
==3499== by 0x825B1DC: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:308)
==3499== by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499== by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499== by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp:625)
==3499== by 0x80AFA7E: sp_file_save_dialog(Gtk::Window&, SPDocument*, Inkscape::Extension::FileSaveMethod) (file.cpp:897)
==3499== by 0xA7D25A3: ???

The other set_param_string lines all generate memory leaks too. So do set_param_bool lines like emf-inout.cpp:338

    ext->set_param_bool("FixPPTCharPos",new_FixPPTCharPos);

==3499== 2 bytes in 1 blocks are definitely lost in loss record 474 of 58,207
==3499== at 0x402BE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==3499== by 0x8519BBE: Inkscape::Util::share_string(char const*, unsigned int) (gc-core.h:77)
==3499== by 0x8519CAE: Inkscape::Util::share_string(char const*) (share.cpp:20)
==3499== by 0x834A4AD: Inkscape::XML::SimpleNode::setAttribute(char const*, char const*, bool) (simple-node.cpp:375)
==3499== by 0x80DEAC2: Inkscape::Preferences::_setRawValue(Glib::ustring const&, char const*) (preferences.cpp:732)
==3499== by 0x81FA69E: Inkscape::Extension::ParamBool::set(bool, SPDocument*, Inkscape::XML::Node*) (bool.cpp:61)
==3499== by 0x825B00A: Inkscape::Extension::Internal::Emf::save(Inkscape::Extension::Output*, SPDocument*, char const*) (emf-inout.cpp:338)
==3499== by 0x8203CF6: Inkscape::Extension::Output::save(SPDocument*, char const*) (output.cpp:218)
==3499== by 0x82026BF: Inkscape::Extension::save(Inkscape::Extension::Extension*, SPDocument*, char const*, bool, bool, bool, Inkscape::Extension::FileSaveMethod) (system.cpp:343)
==3499== by 0x80B0049: _ZL9file_saveRN3Gtk6WindowEP10SPDocumentRKN4Glib7ustringEPN8Inkscape9Extension9ExtensionEbbNS9_14FileSaveMethodE.constprop.345 (file.cpp...

Read more...

~suv (suv-lp) wrote :

> Used "bzr update" to bring my local copy up to the changes ~suv noted in #15.
> Tested the updated lp988601 and it is still broken.

Are you saying that the branch (without any local changes) is broken for you (i..e. fails to compile)? If so, please provide details about what exactly is broken in the branch (not in your local copy - you have to resolve conflicts due to local changes yourself): the branch builds without failure for me on two different systems (without any further local changes except then one for autogen.sh from bug #992047 on Lion).

Kris (kris-degussem) wrote :

In reply to comment 16:
Can we keep this report for the original bug, please? Already five different memory leaks have been fixed as a result of this report, 2 other memory leaks and another bug are also mentioned here, while there is still the original bug to be fixed ...

David Mathog (mathog) wrote :

re #17: It compiles, it just still has memory issues, as shown in #16.
re #18: Sorry, got a little carried away. The set_param_bool() leak should be fixed with set_param_string()though. Seems
likely that all of the set_param_*() leak if these two do.

Kris (kris-degussem) wrote :

Patch of comment 11 committed in rev 11715.
Comment 8 issue 1 might make it into Inkscape 0.49, others will be for later on (require refactoring, that should be done after 0.49 release conforming with the release plan).

Kris (kris-degussem) wrote :

Forked comment 8 issue one in a separate bug report for clarity: bug #1061157

Comment 8 issue 3 is probably fixed in trunk. If not, more information and necessary steps to reproduce this are needed in a separate bug report.

Comment 8 issue 5: not clear on how to reproduce this problem. What are the necessary steps: should be reported in a separate bug report.

Comment 16 issue 2 should be fixed in trunk revision 11731.

Comment 16 issue 3: not sure what teh issue is and where it is located. Regarding the description it is best to look after this after the emf branch is merged into trunk and if the issue is still present to file a separate bug report then.

Original bug report, comment 8 issue 2 and comment 16 issue 1 are the same problem. They need quite some refactoring which can not be done until Inkscape 0.49 is released.

Kris (kris-degussem) wrote :

Fixing this bug is way beyond my level of knowlegde atm. For more information see Inkscape garbage collection system (of with ptr_shared is a part). See Inkscape developers mailings discussion: http://tinyurl.com/cof8l5q

Changed in inkscape:
assignee: Kris (kris-degussem) → nobody
status: In Progress → Triaged
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers