Inkscape crashes if SVG contains an invalid "file:" URI

[Patrick Storz]

When an <image> references a file with an invalid "file:" URI Inkscape crashes instead of ignoring the invalid URI.

Minimal testcase:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink= "http://www.w3.org/1999/xlink">
 <image width="10" height="10" xlink:href="file:invalid"/>
</svg>

The following error is thrown on the console:
"terminate called after throwing an instance of 'Glib::ConvertError'"

Reproduced here with Inkscape 0.92.0 r15299 (64-bit) on Windows 10.
Could someone check if Linux/Mac builds are affected, too?

[Patrick Storz (ede123)]

Note that a file URI should have the form
file://hostname/path/to/file.txt

Or alternatively (hostname=localhost is assumed)
file:///path/to/file.txt

Inkscape seems to crash whenever the "file://hostname/" or "file:///" format is not followed.

[Patrick Storz (ede123)]

Here's another sample file with various absolute and relative "file:" URIs.

Inkscape currently crashes for all of them with the exception of the first two which are the only ones valid according to spec.

While fixing the crash it's worth checking if we can even support them properly, since it's common for software to support them regardless of being non-standard (i.e. Firefox handles all of them fine with the exception of the invalid absolute "file:" URI).

[Patrick Storz (ede123)]

Probably related: bug #1404934

[jazzynico (jazzynico)]

Also reproduced on Xubuntu 16.04, Inkscape 0.91 and lp:inkscape/0.92.x rev. 15347.
GDB trace attached.

[su_v (suv-lp)]

On OS X 10.7.5, crash
- not reproduced with Inkscape 0.47 r22583, 0.48.5 r10040,
- reproduced with Inkscape 0.91 r13725, 0.92.0 r15299,
- reproduced with lp:inkscape/0.92.x r15346

[su_v (suv-lp)]

Based on tests with archived 32bit builds (from Mac OS X 10.5.8):
- not reproduced with rev <= 10196,
- reproduced with rev >= 10201;
it seems likely that the crash as reproduced with the test case from comment 2 was exposed since the initial implementation of the resource manager in r10198:
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/changes/10201
https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/10198

[Patrick Storz (ede123)]

Working on a patch now.
- Fixing the crashing issue is pretty straightforward (see part1).
- I still have to look into getting the malformed links to load

[Patrick Storz (ede123)]

I just attached
- the updated "part1": This simply fixes the crash issue. There was still a logic error in the previous example (which was not exposed, though) which is fixed now.
- a complete "part2": Includes the small fix from "part1" and attempts to load resources from "file:" URIs even if they are not completely standard-conformant.

While working on "part2" I also noticed some issues in the resource-manager.cpp that should be fixed now:
- typo in a Glib::file_test that probably made Inkscape re-evaluate relative references
- fixed paths where not always properly converted to relative paths.

I'd suggest to push "part1" to the 0.92.x stable branch (it's an easy enough fix it shouldn't cause any regressions) and what is now "part2" to trunk (I guess the issues fixed are not important enough to justify backporting / risking regressions?)

[jazzynico (jazzynico)]

Modified part1 patch attached.
The v2 version doesn't compile on Xubuntu 16.04 due to the following error:
---
resource-manager.cpp: In member function ‘std::map<Glib::ustring, Glib::ustring> Inkscape::ResourceManagerImpl::locateLinks(const Glib::ustring&, const std::vector<Glib::ustring>&)’:
/usr/include/glib-2.0/glib/gmessages.h:164:43: error: format not a string literal and no format arguments [-Werror=format-security]
                                __VA_ARGS__)
                                           ^
resource-manager.cpp:237:17: note: in expansion of macro ‘g_warning’
                 g_warning(e.what().c_str());
---

Patch part1-v3 tested successfully on Xubuntu 16.04, lp:inkscape/0.92.x rev. 15347.
Targeting to 0.92.2.

[jazzynico (jazzynico)]

New patch for part2. Note that the compiler error only affects autotools based builds, not cmake, and that part2-v2 compiles correctly on lp:inkscape rev. 15459.

[Patrick Storz (ede123)]

Great, thanks for checking!

[Patrick Storz (ede123)]

"1660142_part1-v3.diff" applied and committed in
http://bazaar.launchpad.net/~inkscape.dev/inkscape/0.92.x/revision/15348

"1660142_part2-v3.diff" applied and committed in
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/15461

[jazzynico (jazzynico)]

@Eduard, did you ask Bryce before committing to the 0.92.x branch? Now that we have reached string freeze (and even a bit before), we're expected to ask him. That's the reason why I targeted to 0.92.2 and not 0.92.1.

[jazzynico (jazzynico)]

I didn't fully test the patch for the trunk. As it is a bit more complex, could you please give more details on how it can be tested?

[Patrick Storz (ede123)]

Yes, I was a good boy and asked for permission. ;-)
I didn't notice you already added the 0.92.2 target before.

As for testing trunk:
Throw any xlink:href at it that you can imagine (preferably "broken" ones) and look if something useful happens .

The "resource manager" is supposed to check if a resource exists and - if not - attempt to resolve the broken file link by searching in all parent directories and in all most recently used directories (as well as their parent directories).

Also it should convert absolute links to relative links where it replaced a broken file link (which works but struggles with "." and ".." in paths - which has nothing to do with my patch, though). Funnily we seem to do this in multiple locations in the code (you just recently fixed another one) - seems we're very thorough...

Regarding possible sources for regressions:
* The new functionality I added is that relative "file:" URIs should work now (e.g. the three images to the right of the testcase in comment #2).
* My refactoring slightly changed behavior but only in a scenario that did not matter before: Links were only converted to relative if the file was not found in the initial location (if it had been found it hadn't been detected as broken, though, so the conditional was irrelevant). Now repaired paths are always converted to relative paths (this can now happen if the href has an invalid format and is therefore rewritten while the file link was correct in principle)
* Besides that there was one typo ("uri" -> "combined") which from reading the code probably caused every relative file link to be identified as broken only to be replaced by the same link again. (I didn't debug that part, though, so it might have behaved slightly different).

[Patrick Storz (ede123)]

Just checked the last bullet point above out of curiosity:
In fact without the typo fixed we initially detect *every* relative file link as "broken" (even if it's not).
However due to an (in principle) superfluous check we always looked at the "broken" link again just to see that it was not broken to start with (therefore no replacement was done).

In trunk we now correctly detect a valid relative file link and do not operate on it at all (therefore also the behavioral change described in bullet point 2 above should be fine).

[jazzynico (jazzynico)]

> Yes, I was a good boy and asked for permission. ;-)

I was just a bit surprised because Bryce usually confirms in the bug report, and I saw no message on the #inkscape-devel IRC channel. Anyway, that's a good thing it's in 0.92.1.

Thanks for your detailed explanations on how to test the complete patch!