StrNumCmp can trigger assertion error/segfault

[Tomasz Wlostowski]

Accidentally discovered while playing with MSVC Kicad build. StrNumCmp does not verify if the iterators (str1, str2) are pointing to a valid string character inside of the compare loop. On VS2017 it causes an immediate assertion failure, on some other platforms it may cause a plain segfault.

One of the 'impossible to reproduce' crashes?

Tom

[Tomasz Wlostowski (twlostow)]

Update: it can also trigger an assertion in isdigit(), as this function does not support unicode, but is used to check such characters.

[Seth Hillbrand (sethh)]

Fixing is a good idea but maybe this is a 6 bug?

This appears to depend on isdigit() checking for the string end() position, which for C++11 is guaranteed to be the \0 character ([1] 21.4.1.5). There doesn't seem to be a way for the iterator to bypass this. But maybe I miss something here?

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

[Wayne Stambaugh (stambaughw)]

This just makes me want to cry. What is the purpose of aLength? A quick grep of the source code shows every call to StrNumCmp() has aLength set to INT_MAX. Why would you even do this? I cannot think of any case were dereferencing an invalid iterator is a good idea. I would prefer we fix this for 5.1 even if it pushes back the release.

[Tomasz Wlostowski (twlostow)]

Hi Wayne,

I don't even understand what this function is supposed to do, otherwise I would've rewritten it. It's surprising how many bugs one can unleash by switching the compiler - I actually did switch to MSVC under Windows to have a debugger that works. Pressing Ctrl-C in MSYS breaks the debugger instead of the application being debugged...

Cheers,
Tom

[Wayne Stambaugh (stambaughw)]

AFAICT, the crux of this function is to sort strings containing numbers. With typical string comparison functions U2 > U10 which for our purposes is not desirable. I'll assign this to myself and finish it up today or tomorrow.

[Tomasz Wlostowski (twlostow)]

Thanks Wayne.

I stumbled upon a bunch of other crashes/assertion failures while using pcbnew/eeschema built with MSVC (my branch no longer needs any MSVC-specific patches/workarounds except for the compiler parameters in CMakeLists.txt, as libcontext now supports MSVC). I'll put them into separate bug reports once I'm sure it's not wx/other libraries' fault.

Cheers,
Tom

[KiCad Janitor (kicad-janitor)]

Fixed in revision 8d26b07f67275113c911a9eaa49e9af0d520eab0
https://git.launchpad.net/kicad/patch/?id=8d26b07f67275113c911a9eaa49e9af0d520eab0

[Wayne Stambaugh (stambaughw)]

StrNumCmp() should no longer try to iterate past the end of the strings.

@Seth, thanks for the test suite. It came in handy. I don't know if there are any other test cases that I may have broken but my changes pass all of the current tests.

[Seth Hillbrand (sethh)]

NP. I'm always nervous changing things that have been in place since the first commit.

[Wayne Stambaugh (stambaughw)]

It make me nervous too. We may have to wait a few more days to tag 5.1.0 just to make sure there isn't something lurking in this change that will bite us. I'm not sure Tom is going to finish up his work this weekend so that may not be an issue other than it will push back the 5.1.0 release another week :(

[Tomasz Wlostowski (twlostow)]

Hi Wayne,

It still causes an assertion failure under MSVC. The isdigit() function can be used only with plain ASCII characters and causes an assert when fed an Unicode (non-ASCII) char. The proper function is iswdigit().

Cheers,
T.

[KiCad Janitor (kicad-janitor)]

Fixed in revision 8a03025ac9f37778c7737b2b79b598fb38140613
https://git.launchpad.net/kicad/patch/?id=8a03025ac9f37778c7737b2b79b598fb38140613

[Seth Hillbrand (sethh)]

Hi Tom- Quick patch here to address that last issue.

[Wayne Stambaugh (stambaughw)]

@Seth, thanks for fixing this.

[Wayne Stambaugh (stambaughw)]

@Seth, I just did a quick grep of the KiCad source code and there are quite a few other places where we are using isdigit() to test a wxString character which will be a wxUniChar in unicode builds.

[Wayne Stambaugh (stambaughw)]

I created a patch to fix all remaining instances of isdigit() comparing wxString characters that I could find. I also checked for some of the other ctype character tests but I didn't find any more cases that were used to test wxString characters. I'm hesitant to push this change just in case it breaks things in new and exciting ways. I think I will hold off until 5.1.0 is released and get some testing before cherry picking it for 5.1.1.