On 24-1-2014 19:04, Martin Owens wrote:

Hi devs,

I've tried out the following patch:

http://paste.ubuntu.com/6809633/

And it's not working. The path which used to work well, it now being overwritten with garbage data before the file can be opened. As if the reference has been freed.

I'm sure it's something simple to do with those consts or gchar types. Any ideas?

Example output:

** (inkscape:4946): WARNING **: Can't open file: /home/doctormo/D\x89 \x99 (doesn't exist)

** (inkscape:4946): WARNING **: Can't get document for referenced URI: inner.svg#bar

If you add functions with strings, don't use char* ! Use std::string. The bug in your case is delicate (if it is what I think it is). It's in this function:

+const gchar *URI::Impl::getFullPath(gchar const *base) const { + const gchar *path = (gchar *)_uri->path; + if(!path) return NULL; + // Calculate the absolute path from an available base + if(base && strncmp(path, "/", 1)!=0) { + path = Glib::build_filename(std::string(base), std::string(path)).c_str(); + } + // TODO: Check existance of file here + return path; +}

The following line:

+ path = Glib::build_filename(std::string(base), std::string(path)).c_str();

creates a std::string object (build_filename does that) as a "temporary". You then call it's c_str() method, which returns a pointer to its internal C-string array. At the end of that statement, the temporary is deleted. So now, path points to a place in memory that has been freed. Advice: rewrite at least the new functions to use std::string. And be very careful when converting from std::string to char*.

Another note: "const char*" means that it points to memory that is constant and should not be changed. This generally means that the pointer is not an "owning" pointer; you can also not free such pointers. In your function, because you new a string there (or at least, that is the intent of what you are doing, i.e., building a new path string), you should return char*, to indicate it is an owning pointer and that the caller has to free it at some point. This all breaks down horribly when you return an owning char* that points to the internals of a std::string...

Cheers, Johan

Hi Martin, (CC to devlist as it is instructive I think of how tricky C-strings can be)

About your recent commit:

In uri.h/.cpp: const std::string URI::getFullPath(std::string const base) const Good usage of const, except for the return type. I think defining the return type const in this case requires the compiler to make an extra string copy, instead of being able to do return-value optimization (RVO). You can also get the base string argument by const&.

In uri-references.cpp: std::string base = std::string(g_strdup(document->getBase())); No need to do g_strdup there, as string-class does that. This line is actually a memory leak at the moment. (the memory allocated by g_strdup is never freed) Solution: std::string base ( document->getBase() );

In document.cpp: const char *path = g_strdup(uri.c_str()); document = createNewDoc(path, false, false, this); I am guessing createNewDoc will not start owning the memory pointed to by path (because it is a const char*!). So this is a memory leak, as the memory pointed to by path is never freed. Solution: document = createNewDoc(uri.c_str(), false, false, this);

Ciao, Johan

On 24-1-2014 19:04, Martin Owens wrote:

Hi devs,

I've tried out the following patch:

http://paste.ubuntu.com/6809633/

And it's not working. The path which used to work well, it now being overwritten with garbage data before the file can be opened. As if the reference has been freed.

I'm sure it's something simple to do with those consts or gchar types. Any ideas?

Example output:

** (inkscape:4946): WARNING **: Can't open file: /home/doctormo/D\x89 \x99 (doesn't exist)

** (inkscape:4946): WARNING **: Can't get document for referenced URI: inner.svg#bar

Martin,

---

CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.cl... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel