OCE 0.18.2 segfaults with IGES model

[Nick Østergaard]

This is with oce 0.18.2-2 on archlinux. I can make it render some models, but I think this single model makes it crash. Attached is a test project which should segfault when opening any 3D view that loads that model. The big 3d-viewer or the previewer or 3D properties viewer.

Snipper of backtrace, full backtrace in the attached zip file.
#0 0x00007f9a3481e65e in BRepGProp_Face::Load(TopoDS_Edge const&) () at /opt/oce/lib/libTKTopAlgo.so.11
#1 0x00007f9a348255c6 in BRepGProp_Gauss::Compute(BRepGProp_Face&, BRepGProp_Domain&, gp_Pnt const&, double&, gp_Pnt&, gp_Mat&) ()
    at /opt/oce/lib/libTKTopAlgo.so.11
#2 0x00007f9a34826e05 in BRepGProp_Sinert::Perform(BRepGProp_Face&, BRepGProp_Domain&) () at /opt/oce/lib/libTKTopAlgo.so.11
#3 0x00007f9a3481bdc5 in BRepGProp::SurfaceProperties(TopoDS_Shape const&, GProp_GProps&) () at /opt/oce/lib/libTKTopAlgo.so.11
#4 0x00007f9a34c0c34d in ShapeAnalysis_Wire::CheckSmallArea(TopoDS_Wire const&, bool) () at /opt/oce/lib/libTKShHealing.so.11
#5 0x00007f9a34c399cb in ShapeFix_Face::FixSmallAreaWire(bool) () at /opt/oce/lib/libTKShHealing.so.11
#6 0x00007f9a34c4840b in ShapeFix_Face::Perform() () at /opt/oce/lib/libTKShHealing.so.11
#7 0x00007f9a34c6da9e in ShapeFix_Shape::Perform(Handle_Message_ProgressIndicator const&) () at /opt/oce/lib/libTKShHealing.so.11
#8 0x00007f9a34c6de90 in ShapeFix_Shape::Perform(Handle_Message_ProgressIndicator const&) () at /opt/oce/lib/libTKShHealing.so.11
#9 0x00007f9a34d019c6 in () at /opt/oce/lib/libTKShHealing.so.11
#10 0x00007f9a34cfe8aa in ShapeProcess::Perform(Handle_ShapeProcess_Context const&, char const*) () at /opt/oce/lib/libTKShHealing.so.11
#11 0x00007f9a35101a86 in XSAlgo_AlgoContainer::ProcessShape(TopoDS_Shape const&, double, double, char const*, char const*, Handle_Standard_Transient&, Handle_Message_ProgressIndicator const&) const () at /opt/oce/lib/libTKXSBase.so.11
#12 0x00007f9a3869b221 in IGESToBRep_Actor::Transfer(Handle_Standard_Transient const&, Handle_Transfer_TransientProcess const&) ()
    at /opt/oce/lib/libTKIGES.so.11
#13 0x00007f9a3502873f in Transfer_ActorOfTransientProcess::Transferring(Handle_Standard_Transient const&, Handle_Transfer_ProcessForTransient const&) () at /opt/oce/lib/libTKXSBase.so.11
#14 0x00007f9a3503a5a2 in Transfer_ProcessForTransient::TransferProduct(Handle_Standard_Transient const&) () at /opt/oce/lib/libTKXSBase.so.11
#15 0x00007f9a3503bcff in Transfer_ProcessForTransient::Transferring(Handle_Standard_Transient const&) () at /opt/oce/lib/libTKXSBase.so.11
#16 0x00007f9a3503c498 in Transfer_ProcessForTransient::Transfer(Handle_Standard_Transient const&) () at /opt/oce/lib/libTKXSBase.so.11
#17 0x00007f9a350bebd6 in XSControl_TransferReader::TransferOne(Handle_Standard_Transient const&, bool) () at /opt/oce/lib/libTKXSBase.so.11
#18 0x00007f9a350b9650 in XSControl_Reader::TransferRoots() () at /opt/oce/lib/libTKXSBase.so.11
#19 0x00007f9a3e33c729 in IGESCAFControl_Reader::Transfer(Handle_TDocStd_Document&) () at /opt/oce/lib/libTKXDEIGES.so.11
#20 0x00007f9a3ebf46f0 in readIGES(Handle_TDocStd_Document&, char const*) (m_doc=..., fname=0x5611b1dbb5b0 "/home/nickoe/test_proj/User Library-Fuse_Holder.IGS") at /home/nickoe/kicad-source-mirror/plugins/3d/oce/loadmodel.cpp:415
#21 0x00007f9a3ebf49d8 in LoadModel(char const*) (filename=0x5611b1dbb5b0 "/home/nickoe/test_proj/User Library-Fuse_Holder.IGS")

Application: kicad
Version: (2017-12-18 revision 3c6d17026)-master, debug build
Libraries:
    wxWidgets 3.0.3
    libcurl/7.57.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4 libpsl/0.18.0 (+libidn2/2.0.4) libssh2/1.8.0 nghttp2/1.28.0
Platform: Linux 4.14.6-1-ARCH x86_64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.3 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.65.1
    Curl: 7.57.0
    Compiler: GCC 7.2.1 with C++ ABI 1011

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

[Kevin Cozens (qq3dh7wn6set-fehmn-mm0v6n6x10rb)]

I loaded up the project in KiCad built from git master and I was not able to reproduce the problem. In case it makes a different I should point out I only have oce 0.17.1 installed on my machine.

[Nick Østergaard (nickoe)]

I think I have only seen this issue with oce 0.18.2, it also crashes on another arch box with that oce verstion. And I am sure it has worked previously. I will attempt to downgrade oce.

[Nick Østergaard (nickoe)]

I have just tested with oce 0.17.2 on the same machine and here there is no segfault when loading the model configured in the example project.

[Tomasz Wlostowski (twlostow)]

Should we mark the bug as invalid? It seems to be a problem with oce, not Kicad.

[Eldar Khayrullin (eldar)]

The status should be 'Wont fix' I think

[Nick Østergaard (nickoe)]

I don't think it should be marked as invalid yet. Potentially it could be because we provide bad data to the OCE call. But it may be appropriate to open a bug against OCE. I will do that soon.

[Nick Østergaard (nickoe)]

I reported this issue upstream for OCE at:
https://github.com/tpaviot/oce/issues/687

[Nick Østergaard (nickoe)]

I will unlink the milestone from rc1 since this is unlikely to get debugged right now.

[CKO (ckoller)]

Latest Kicad on arch with oce 0.18.2-2 built as shown in
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=oce
segfaults upon opening of the 3d viewer.

It seems to crash in:
-> error 4 in libTKTopAlgo.so.11.0.0.

[Wayne Stambaugh (stambaughw)]

I thought this was fixed recently by adding a missing dependency library? I would like to get this fixed before v5 rc1.

[Maciej Suminski (orsonmmz)]

I have just checked and it still segfaults. I added a minimal example to reproduce the crash to the OCE bug report nickoe had created. I am not sure if we can do much about it, perhaps there are some configuration options we could try, but I am not familiar with oce.

[Wayne Stambaugh (stambaughw)]

We need to come up with some kind of resolution for this. I don't like
the idea of releasing a stable version with a know segfault. At the
very least we could refuse to build the plugin against oce 18.2. That
would be ugly but it's better than a segfault.

On 02/17/2018 09:33 AM, Maciej Suminski wrote:
> I have just checked and it still segfaults. I added a minimal example to
> reproduce the crash to the OCE bug report nickoe had created. I am not
> sure if we can do much about it, perhaps there are some configuration
> options we could try, but I am not familiar with oce.
>

[Maciej Suminski (orsonmmz)]

As far as I understand, it is a corner case that leads to a crash. I think most distros ship oce 0.17, so they are safe. I suppose Arch (and derived distros) users might be affected by the problem, maybe a warning would be enough for the moment.

[Maciej Suminski (orsonmmz)]

I stand corrected, oce 0.18 is provided by a few major distros (openSUSE, Fedora), but still it is a corner case.

[Wayne Stambaugh (stambaughw)]

It's also in Debian testing which means that it will appear in the next
stable version of Debian and most of it's derivatives which isn't much
of a corner case.

On 2/17/2018 2:58 PM, Maciej Suminski wrote:
> I stand corrected, oce 0.18 is provided by a few major distros
> (openSUSE, Fedora), but still it is a corner case.
>

[Maciej Suminski (orsonmmz)]

I meant it does not crash with every IGES model, but with this particular file. Perhaps I am mistaken about the corner case part, it is hard to say whether it is a common condition occurring in IGES files that leads to a crash.

[Wayne Stambaugh (stambaughw)]

I see you sent a test project to the oce devs. Thanks. Maybe they will give us an idea how to resolve the issue or fix their bug. Until then, it looks like the fix for this is out of our hands. Does this bug occur using OpenCascade? If not, that may be an alternate solution although I don't know how common it is for distros to package OpenCascade.

[Seth Hillbrand (sethh)]

This is apparently caused by the OCE 0.18 IGES processor not handling small faces whose points are all degenerate at the precision specified. This does not appear to be an issue in OCCT 6.9.1, which OCE 0.18 is based on.

KiCad specifies a hard-coded precision that is pretty coarse, potentially leading to the issue for many files. It does not appear to affect similar STEP files with small faces.

We can partially work around it by following the IGES precision specifier in each file. This leaves the issue of not creating degenerate faces to the IGES generator, which is better than the current situation and might need to be sufficient until the OCE maintainer fixes the underlying issue.

[Maciej Suminski (orsonmmz)]

Seth, thank you for the patch. I confirm it fixes the segfault and I plan to merge your changes. The last thing I need to test is to try to wrap the precision settings in #ifdefs to make them dependent on OCE version. I believe Cirillo has set the precision for a reason (probably performance).

I also tested just released 0.18.3-1, but the segfault is still occurring there.

[Wayne Stambaugh (stambaughw)]

Do the precision settings in the IGES files not work correctly with
0.17.2 and older versions of oce? I would like to avoid #ifdefs if
possible. @Cirillo, can you provide any guidance as why you chose to
set the precision in code instead of using the precision provided in
IGES files?

On 2/20/2018 7:34 AM, Maciej Suminski wrote:
> Seth, thank you for the patch. I confirm it fixes the segfault and I
> plan to merge your changes. The last thing I need to test is to try to
> wrap the precision settings in #ifdefs to make them dependent on OCE
> version. I believe Cirillo has set the precision for a reason (probably
> performance).
>
> I also tested just released 0.18.3-1, but the segfault is still
> occurring there.
>

[Maciej Suminski (orsonmmz)]

Seth's patch works fine with 0.17.2 as well. Cirillo has put a comment regarding the precision setting "default 0.0001 has too many triangles", so it looks like a performance issue to me.

[Seth Hillbrand (sethh)]

By default, the precision in the file will not merge any segments, so we're stuck with whatever the precision is and very dependent on the model creator to not make excessively detailed files.

If we wanted to compromise, this _particular_ file has a small face (embedded in a small face) that is ~ 0.11mm x 0.12mm, so setting the minimum size to 0.10 (instead of 0.14) will fix the segfault for this file and keep the most of the performance Cirillo wanted. But there are no guarantees for the next file after that.

[Wayne Stambaugh (stambaughw)]

So it comes down to the following:

1. Do we using the precision specified in the file or do we override it
to reduce the number of triangles to improve performance.

2. If we choose to override the precision, is there a precision that
doesn't crash oce 0.18.2 and higher that still works well with oce 0.17
and lower.

3. If there is no precision that works for all versions of oce, do we
use some type of oce version checking (static using #ifdef/#endif or
runtime) to select an appropriate precision.

4. If the version check is too ugly, loop back to 1.

I prefer #1 if the performance hit is not too significant. It may be
that we have to live with #3 if #2 isn't viable.

On 2/20/2018 9:19 AM, Maciej Suminski wrote:
> Seth's patch works fine with 0.17.2 as well. Cirillo has put a comment
> regarding the precision setting "default 0.0001 has too many triangles",
> so it looks like a performance issue to me.
>

[Wayne Stambaugh (stambaughw)]

Could we use a precision based on the area of the face with a minimum
precision for larger faces? We could try the cheap and dirty fix of
setting the precision to 0.1 and hope for the best. At least we know
where the issue lies and how to fix it should it be necessary.

On 2/20/2018 9:39 AM, Seth Hillbrand wrote:
> By default, the precision in the file will not merge any segments, so
> we're stuck with whatever the precision is and very dependent on the
> model creator to not make excessively detailed files.
>
> If we wanted to compromise, this _particular_ file has a small face
> (embedded in a small face) that is ~ 0.11mm x 0.12mm, so setting the
> minimum size to 0.10 (instead of 0.14) will fix the segfault for this
> file and keep the most of the performance Cirillo wanted. But there are
> no guarantees for the next file after that.
>

[Maciej Suminski (orsonmmz)]

The tricky part is you need to set the precision before reading the file, so you will not know the face sizes before you read the model file. I am afraid that once we pick a precision setting, it will work for certain files, but then we will find another one model that needs a different value.

Frankly, this needs to be fixed in OCE. I understand the library may not handle particular cases, but there are more gentle ways to signal it then a segfault. If we received an error, we would simply adjust the precision value and try again.

[Maciej Suminski (orsonmmz)]

Strange, I cannot reproduce the crash with oce 0.18.2 and 0.18.3 anymore, but as far as I know there were no changes to the IGES plugin.

[Seth Hillbrand (sethh)]

One option might be to load the model with the file precision, then check the faces as Wayne suggested before healing.

However, this is getting pretty tweaky as I'm not sure of a way to check before healing whether two faces are degenerate, so we might end up hitting the same bug.

The other option is to selectively disable this type of face healing in the IGES import. We can control this using an external configuration file. I haven't found a way to control the import settings using online commands only.

If you'd prefer a patch that handles this using config files, I can work that out. Otherwise, I'd be inclined to keep the file precision. For the model given, I'm not seeing a difference in face count in the resulting cascade model between a precision of 0.1 (maximum before crashing) and using the file precision (1e-8 for this model), which indicates to me that there is no healing in the given model before segfaulting.

@Orson- I agree with you that this should be fixed in OCE. Unfortunately, OCE appears to be a lightly-maintained project, with a few different crash bugs remaining open (some for years). Looks like if we want this fixed in OCE, we would need to patch it ourselves. Even then, it might not be merged (c.f. https://github.com/tpaviot/oce/issues/607)

[Wayne Stambaugh (stambaughw)]

I don't have a preference as to the solution. I just want to be sure
that whatever solution we choose it gives us the most stability.

On a related note, Seth, whatever happen to the oce/opencascade
dependency patch? Is that ready to go or have you abandoned that approach.

On 02/21/2018 01:51 PM, Seth Hillbrand wrote:
> One option might be to load the model with the file precision, then
> check the faces as Wayne suggested before healing.
>
> However, this is getting pretty tweaky as I'm not sure of a way to check
> before healing whether two faces are degenerate, so we might end up
> hitting the same bug.
>
> The other option is to selectively disable this type of face healing in
> the IGES import. We can control this using an external configuration
> file. I haven't found a way to control the import settings using online
> commands only.
>
> If you'd prefer a patch that handles this using config files, I can work
> that out. Otherwise, I'd be inclined to keep the file precision. For
> the model given, I'm not seeing a difference in face count in the
> resulting cascade model between a precision of 0.1 (maximum before
> crashing) and using the file precision (1e-8 for this model), which
> indicates to me that there is no healing in the given model before
> segfaulting.
>
> @Orson- I agree with you that this should be fixed in OCE.
> Unfortunately, OCE appears to be a lightly-maintained project, with a
> few different crash bugs remaining open (some for years). Looks like if
> we want this fixed in OCE, we would need to patch it ourselves. Even
> then, it might not be merged (c.f.
> https://github.com/tpaviot/oce/issues/607)
>
> ** Bug watch added: github.com/tpaviot/oce/issues #607
> https://github.com/tpaviot/oce/issues/607
>

[Seth Hillbrand (sethh)]

The alternative idea is to process the IGES file as normal but we would need to use the attached configuration file to set the processing parameters (It disables the FixFreeFaceMode). This should allow us to keep the tolerance level where it is for all of the other FixShape steps but avoid the segfault.

In order to tell OCE to look for it, we need to set an environmental variable (CSF_IGESDefaults) to the path where we keep it. We could keep it in the KiCad common directory. It doesn't really fit in any of the existing subdirectories. Maybe template? Do you have a preference?

[Wayne Stambaugh (stambaughw)]

I'm assuming by kicad common directory you mean the
install_path/share/kicad folder? This will work on linux and windows.
I'm not so sure about macos. We could add the environment variable with
a reasonable default in the common/pgm_base.cpp file. This will not
work in all cases but should work for most. This will add another
environment variable to the windows installer as well. Can any of our
macos devs comment about the location of this config file on macos.

On 2/21/2018 8:30 PM, Seth Hillbrand wrote:
> The alternative idea is to process the IGES file as normal but we would
> need to use the attached configuration file to set the processing
> parameters (It disables the FixFreeFaceMode). This should allow us to
> keep the tolerance level where it is for all of the other FixShape steps
> but avoid the segfault.
>
> In order to tell OCE to look for it, we need to set an environmental
> variable (CSF_IGESDefaults) to the path where we keep it. We could keep
> it in the KiCad common directory. It doesn't really fit in any of the
> existing subdirectories. Maybe template? Do you have a preference?
>
> ** Attachment added: "IGES"
> https://bugs.launchpad.net/kicad/+bug/1738872/+attachment/5060050/+files/IGES
>

[Nick Østergaard (nickoe)]

@Seth, I confirm as Maceij that the patch you provided earlier seems to fix the issue. When I do not have your patch applied and use use the OCE config file you provided in #30, then it will also make another IGES model on the design to dissapear.

[Seth Hillbrand (sethh)]

@Nick-

Thanks for testing that. Is it any other IGES model or a specific one?

[Maciej Suminski (orsonmmz)]

@Nick, ping.

@Seth, Wayne
It seems that leaving the default precision setting is the safest option here. Perhaps we should actually check if there is any noticable performance drop when we do not lower the precision.

[Wayne Stambaugh (stambaughw)]

On 4/4/2018 3:52 AM, Maciej Suminski wrote:
> @Nick, ping.
>
> @Seth, Wayne
> It seems that leaving the default precision setting is the safest option here. Perhaps we should actually check if there is any noticable performance drop when we do not lower the precision.
>

A performance hit is better than a segfault. If the performance hit is
too great, we can try to fix that later.

[Seth Hillbrand (sethh)]

Agreed. I'll push the patch setting the value to file-controlled.

[KiCad Janitor (kicad-janitor)]

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