wolph / numpy-stl

Simple library to make working with STL files (and 3D objects in general) fast and easy.
http://numpy-stl.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
624 stars 105 forks source link

Perform a more thorough check on ASCII STL files. #80

Closed spotrh closed 6 years ago

spotrh commented 6 years ago

I came across a binary STL file which had "solid" in the first 80 characters (can provide on request). This causes the current code to segfault, when it tries to treat a binary STL as an ASCII STL.

My friends at Lulzbot have a patch that they applied to try to fix this issue: https://code.alephobjects.com/rNUMPYSTL15d9853f7769a0d3c5318ff75cd7b7481aa1af47

However, their patch causes your test suite to fail (specifically, the ASCII test on long part names), so I investigated further.

The test fails because numpy-stl wants to support ASCII files with long part name identifier strings (the part that comes after "solid"). The STL standard exists, but isn't published anywhere, and I was unable to determine if it specifies a max length for the part name identifier strings. (I've emailed 3D Systems to ask for the standard doc, but I'm not holding my breath).

numpy-stl reads in 80 characters as a header, because the binary specification says the 80 chars are header. The code assumes it only has the 80 character header when operating on binary STLs later, so we can't just read more without breaking lots of things. We also don't know how much more we need to read.

The Lulzbot change adds a search of the first 80 character header for "facet normal", which fixes the crash from my weird binary STL file, but with the test case in your test suite for long part name, "facet normal" does not appear in the first 80 characters, and the test fails (because it thinks the ASCII STL is a binary).

So how can we check for a weird binary STL like mine, which has "solid" in the first 80 chars AND pass the long part name test case?

We know that an ASCII STL file will end with "endsolid" on the last line. But the last line can either look like this:

endsolid

or

endsolid part name string of unknown length

And of course, if it is a binary STL file, it will not have "lines", just data.

Here's how I did it.

I found evidence that the smallest possible valid STL file is 186 bytes (https://github.com/WoLpH/numpy-stl/issues/37). So, assuming that no one is using ASCII files with part name identifier strings longer than 176 chars, read in 176 + 10 ("endsolid" + " " + "176 char ident string" + "\n") chars, and search those chars for "endsolid". If we find it, then it's legit ASCII. If not, it's either binary, an ASCII file with absurdly long part name identifiers, or corrupt.

Since we can seek to the end of the file without reading in the whole file, we can get the "tail" relatively quickly.

This catches the broken binary file, and passes the test suite.

Now, if we discover that there is a max length for this part name identifier string, the problem becomes much easier to solve, but I strongly suspect that:

A) There is not. B) Even if the standard says there is, there are ASCII STL files which ignore it and have longer than spec strings.

Thankfully, there are very few ASCII STL files in use, and even fewer with very long part name strings, so hopefully the corner case here has become so small that no one will fall in it for a very very very long time (if ever).

spotrh commented 6 years ago

Argh. I see that this doesn't work everywhere, because older python is picky about how to seek backwards. I suppose I could read a long "head" that is 186 chars long and see if "facet normal" appears, but I felt that it was much more likely for that to fail if the part name identifier string is long (it seems much more common for that string to be after "solid" than "endsolid").

spotrh commented 6 years ago

The other issue is that your test for "incomplete ASCII" file is much too small for this assumption. :/

spotrh commented 6 years ago

Okay, the "long head" approach works, but several of your test cases are too small for that assumption. If I add an assert to check that we got a full "long head", the tests pass, but there are coverage gaps that I can't figure out how to close. Not sure where to go from here. :/

wolph commented 6 years ago

Can you upload the STL file that breaks the system? I'd like to see what we can do about it.

By default the system actually falls back to reading the data as binary is reading it as ascii failed for some reason: https://github.com/WoLpH/numpy-stl/blob/cede381a6bac779e36565907f1e3dcf18b79f8e2/stl/stl.py#L76-L91

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at ?% when pulling 8722fcf6890f497450a0ed4a0cdda5444cb958b6 on spotrh:develop into cede381a6bac779e36565907f1e3dcf18b79f8e2 on WoLpH:develop.

spotrh commented 6 years ago

Here is the tricky file: http://spot.fedorapeople.org/rear_case.stl

Uvar commented 6 years ago

I get the issue, however...version 2.3.2 has no issues whatsoever, in reading the "rear_case.stl". If it causes a segfault now, time to dig into what changed. :)

wolph commented 6 years ago

It seems the issue only occurs when the speedups are enabled so it's yet another one of these hard to trace issues :(

For most cases the speedups are negligible and cause more harm than good (such as this issue) which is why it's disabled by default. But at least there's a reproducible bug now so perhaps the issue can be fixed once and for all :)

wolph commented 6 years ago

@spotrh can you test the new development release to see if it fixes your issue?

spotrh commented 6 years ago

Confirmed. Latest code resolves the segfault. Thanks for the quick fix!