StrNumCmp can trigger assertion error/segfault

Bug #1818157 reported by Tomasz Wlostowski on 2019-02-28
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
KiCad
High
Wayne Stambaugh

Bug Description

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

Changed in kicad:
status: New → Confirmed
importance: Undecided → High
milestone: none → 5.1.0
Tomasz Wlostowski (twlostow) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Changed in kicad:
assignee: nobody → Wayne Stambaugh (stambaughw)
Tomasz Wlostowski (twlostow) wrote :

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) wrote :

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

Changed in kicad:
status: Confirmed → Fix Committed
Wayne Stambaugh (stambaughw) wrote :

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) wrote :

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

Wayne Stambaugh (stambaughw) wrote :

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) wrote :

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.

Changed in kicad:
status: Fix Committed → Confirmed
KiCad Janitor (kicad-janitor) wrote :

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

Changed in kicad:
status: Confirmed → Fix Committed
assignee: Wayne Stambaugh (stambaughw) → Seth Hillbrand (sethh)
Seth Hillbrand (sethh) wrote :

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

Changed in kicad:
assignee: Seth Hillbrand (sethh) → Wayne Stambaugh (stambaughw)
Wayne Stambaugh (stambaughw) wrote :

@Seth, thanks for fixing this.

Wayne Stambaugh (stambaughw) wrote :

@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) wrote :

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.

Changed in kicad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers