Segmentation fault after a failed assertion when "Cleanup Tracks and Vias" in Pcbnew

[Paul van der hoeven]

KiCad V5.1.0+dfsg1 gets a "failed assertion" when clicking on:
> Pcbnew / Edit / Cleanup Tracks and Via's.

The backtrace shows:
ASSERT INFO:
/build/kicad-PCtFts/kicad-5.1.0+dfsg1/pcbnew/tracks_cleaner.cpp(363): assert "connectivity->GetConnectivityAlgo()->ItemEntry( aTrack ).GetItems().size() != 0" failed in testTrackEndpointDangling().

BACKTRACE:
[1] wxEvtHandler::ProcessEventIfMatchesId(wxEventTableEntryBase const&, wxEvtHandler*, wxEvent&)
[2] wxEventHashTable::HandleEvent(wxEvent&, wxEvtHandler*)
[3] wxEvtHandler::TryHereOnly(wxEvent&)
[4] wxEvtHandler::DoTryChain(wxEvent&)
[5] wxEvtHandler::ProcessEvent(wxEvent&)
[6] wxWindowBase::TryAfter(wxEvent&)
[7] wxEvtHandler::SafelyProcessEvent(wxEvent&)
[8] wxMenuBase::SendEvent(int, int)
[9] g_closure_invoke
[10] g_signal_emit_valist
[11] g_signal_emit
[12] gtk_widget_activate
[13] gtk_menu_shell_activate_item
[14] g_signal_emit_valist
[15] g_signal_emit
[16] gtk_main_do_event
[17] g_main_context_dispatch
[18] g_main_loop_run
[19] gtk_main
[20] wxGUIEventLoop::DoRun()
[21] wxEventLoopBase::Run()
[22] wxAppConsoleBase::MainLoop()
[23] wxNavigationEnabled<wxTopLevelWindow>::RemoveChild(wxWindowBase*)
[24] wxEntry(int&, wchar_t**)
[25] wxNavigationEnabled<wxTopLevelWindow>::RemoveChild(wxWindowBase*)
[26] __libc_start_main
[27] _start

**** Full version info: ****

Application: kicad
Version: 5.1.0+dfsg1-1, release build
Libraries:
    wxWidgets 3.0.4
    libcurl/7.64.0 OpenSSL/1.1.1b zlib/1.2.11 libidn2/2.0.5 libpsl/0.20.2 (+libidn2/2.0.5) libssh2/1.8.0 nghttp2/1.36.0 librtmp/2.3
Platform: Linux 4.19.0-4-amd64 x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.4 (wchar_t,wx containers,compatible with 2.8) GTK+ 3.24
    Boost: 1.67.0
    OpenCASCADE Community Edition: 6.9.1
    Curl: 7.64.0
    Compiler: Clang 7.0.1 with C++ ABI 1002

Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=ON
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_PYTHON3=ON
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_WXPYTHON_PHOENIX=ON
    KICAD_SCRIPTING_ACTION_MENU=ON
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=ON
    KICAD_USE_OCC=OFF
    KICAD_SPICE=ON

****

On my system this is triggered every time in the "Highpower-Mechaduino" project.
I zipped the whole project and uploaded it to the KiCad users forum:
https://forum.kicad.info/t/kicad-5-1-crashes-often/16232/13?u=paulvdh

Direct link to the zipped project:

https://kicad-info.s3.dualstack.us-west-2.amazonaws.com/original/2X/9/9b29de1e9c5628c4c063f82109693d1a326f5645.zip

[Wayne Stambaugh (stambaughw)]

@Tom, what is the status of this bug? I would like to git 5.1.3 out soon and this is the only critical bug report.

[KiCad Janitor (kicad-janitor)]

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

[Michael Kavanagh (michaelkavanagh)]

I think this needs cherry-picking onto 5.1.

[Wayne Stambaugh (stambaughw)]

@Michael, this commit does not cherry pick cleanly. It looks like there have been major incompatible changes to pcbnew/tracks_cleaner.cpp. Either someone forgot to cherry-pick previous commits or the entire track cleaning code has been changed in a way that is not compatible with 5.1.

@Tom, would you please take a look at the 5.1 branch and see what needs to be done to fix this issue in the 5.1 branch?

[Jeff Young (jeyjey)]

Cleanup tracks and vias was re-written for 6.0 to have the dry-run feature.

That being said, it looks like Tom's fix is a one-liner, so it shouldn't be too hard to apply it to the other code.

[Wayne Stambaugh (stambaughw)]

@Jeff, it's not hard to apply I'm just not sure it's correct given the magnitude of the changes.

[Jeff Young (jeyjey)]

@Wayne, agreed. It's not clear to me if the bug existed in the old algorithm or was introduced in the re-write. (My guess is that the bug exists in 5.1, but I haven't looked into it deeply.)

[Seth Hillbrand (sethh)]

@Wayne-

The bug was introduced in the dev branch. No need to cherry-pick

[Seth Hillbrand (sethh)]

*fixed* (not introduced) in the dev branch.

That'll teach me to look at bug reports before coffee.

[Wayne Stambaugh (stambaughw)]

Just so I'm clear, does this need fixed in 5.1? It appears to me that it does.

[Seth Hillbrand (sethh)]

No. The fix exists in both branches already.

5.1:
https://github.com/KiCad/kicad-source-mirror/blob/5.1/pcbnew/tracks_cleaner.cpp#L425

dev:
https://github.com/KiCad/kicad-source-mirror/blob/master/pcbnew/tracks_cleaner.cpp#L309

[Wayne Stambaugh (stambaughw)]

@Seth, it's not as fixed as you would like it to be. It still crashes for me 100% of the time using the project provided by the bug reporter. It looks like there might be a corner case which this project nicely provides. I have a fix but I'm not sure it is strictly correct. I changed the assert to a logical if statement so when nothing is connected to the track passed to the connectivity algorithm dangling track test it just returns true. I'm not entirely sure why this was considered an invalid condition and an assert was used but I'm not familiar with this code so maybe I'm interpreting what this is supposed to do. It seems to me that it is entirely plausible that you could have a track segment that is completely orphaned which would trigger this bug. If you see an issue with my fix (see patch) please let me know.

[Seth Hillbrand (sethh)]

"It's not as fixed as you would like it to be" will be the epitaph on my tombstone. :/

I can't recreate the crash on 5.1, so I'm not sure what I'm doing differently.

I'm fine with the general patch (we do something similar in master) although, I would prefer that we return false as we don't want to touch tracks that are not in the connectivity system.

[Wayne Stambaugh (stambaughw)]

On 6/28/19 11:47 AM, Seth Hillbrand wrote:
> "It's not as fixed as you would like it to be" will be the epitaph on my
> tombstone. :/

I feel your pain (said out loud with my be Bill Clinton impersonation) ;)

>
> I can't recreate the crash on 5.1, so I'm not sure what I'm doing
> differently.

Maybe we broke something along the way. I get the crash on
(5.1.2-166-g3b038feae) using the archive posted to this bug report.

>
> I'm fine with the general patch (we do something similar in master)
> although, I would prefer that we return false as we don't want to touch
> tracks that are not in the connectivity system.
>

That's fine although I'm not sure that is what users will expect. If a
track has no connectivity information, doesn't that qualify as a
dangling track? It's not clear to me what is expected. Maybe we need
to beef up the connectivity algo doxygen comments. In any event, I will
change the return to false and push the fix.

[Wayne Stambaugh (stambaughw)]

Fixed in the 5.1 branch.

https://git.launchpad.net/kicad/commit/?id=574473c480efdae5a6a5fbecda4a0dc8d9b02ddb

[Seth Hillbrand (sethh)]

I finally got the issue!

The root cause is that there is an invalid track (line 6235) in the board. The first segment is in the Dwg.User layer, which is never in the connectivity database (therefore we get the null-deference when cleaning) but is still added to the Tracks DLIST.

I think that the correct action for safely (ish) falling back from an assert is to not touch the data because we really don't know what it is or why it is invalid.

But the basic fix for this issue is to drop invalid tracks on non-copper layers when loading the board. Any objections to this approach. It would apply to master as well.

[Jeff Young (jeyjey)]

+1

[Wayne Stambaugh (stambaughw)]

@Seth, I don't have an issue with your fix but I am concerned that a track somehow ended up on a non-copper layer. Is this even possible in Pcbnew or is it possible that this board was modified with a Python script or by hand? If Pcbnew allowed this, we need to track down that bug as well. We should not allow tracks on non-copper layers.

[Seth Hillbrand (sethh)]

I tried for a bit with 5.1 but could get a track to change without resorting to python.

I note that the OP's board is from v4, so the bug may have been present in an earlier version.

I'll push the drop fix later tonight.