View Issue Details

IDProjectCategoryView StatusLast Update
0003070File formatsGeneralpublic2021-02-06 06:31
ReporterKrishty Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status confirmedResolutionopen 
Target Version0.20 
Summary0003070: VRML 2 crashes with corner cases
Description1. FreeCAD crashes whenever VRML files contain recursive structures.
2. It crashes after some time if files contain very long names.

Recursive structures are not allowed in VRML, and they should not appear in the wild. However, FreeCAD shouldn't just crash either. Same for long names.
Steps To Reproduce1. load one of the attached files with "crash" prefix
2. crash immediately

OR

1. click Open File
2. select the file with "corrupts memory" prefix
3. Open
4. file seems to load file
5. click Open File again
6. FreeCAD crashes a fraction of a second after that (which indicates a memory corruption)
Additional Informationrunning 0.16 on Windows 7
TagsVRML
FreeCAD Information

Activities

Krishty

2017-06-05 01:02

reporter  

Krishty

2017-06-05 01:10

reporter   ~0009285

same crashes with VRML 1 recursive structures & VRML 1 long name

Kunda1

2017-06-10 11:35

administrator   ~0009331

Discussion thread: https://forum.freecadweb.org/viewtopic.php?f=8&t=22875

wmayer

2017-06-11 12:16

administrator   ~0009343

<quote>
2. It crashes after some time if files contain very long names.
</quote>
That's a bug in the Coin library. Inside the class SbName the function find_string_address is called (over several steps) and this raises an assert if the string is longer than 65004 characters. Many systems already crash there (when Coin was built with the debug option). On Windows in release mode it crashes at a later point because the internal string buffer seems to be corrupted.

Does the VRML standard say anything about the maximum length of a name?

Krishty

2017-06-11 12:33

reporter   ~0009344

The VRML 2 standard defines a limit of 50 Bytes per name:
http://tecfa.unige.ch/guides/vrml/vrml97/spec/part1/conformance.html#7.3.3

VRML 1 standard does not mention anything.

I recommend aborting after 50 B if you’re strict or after 127 B if you want to avoid problems with 3rd party libraries.

wmayer

2017-06-11 12:46

administrator   ~0009345

Thanks for the link.
> I recommend aborting after 50 B if you’re strict or after 127 B if you want to avoid problems with 3rd party libraries.

Currently we rely on Coin's VRML importer here. So as a workaround we must implement our own parser that verifies the DEF's of a VRML file.

Krishty

2017-06-11 13:01

reporter   ~0009346

Yes, a separate verification pass would also solve the recursive DEF/USE crash. However, given that VRML 1 & 2 are vastly different in grammar and the bug exists in both versions, that’s probably TWO parsers to be written.

It should definitely be reported to the COIN maintainers and they need to fix that.

wmayer

2017-06-11 14:49

administrator   ~0009347

Last edited: 2017-06-11 15:36

I pushed a fix https://github.com/FreeCAD/FreeCAD/commit/a405b4dae05fc55d784cc485723f278b83ea64ee the solves the crash when loading the file crash - recursive USE.wrl Having this function inside FreeCAD makes sense because cyclic graphs can also happen outside the scope of reading in a file.

The fix doesn't prevent FreeCAD from crashing when reading in crash - recursive PROTO.wrl because there is already a stack overflow during the read process.

Btw, MeshLab handles the cases with recursive PROTO and too long names but crashes with recursive USE. Blender handles all three cases.

Krishty

2017-06-11 18:09

reporter   ~0009348

> Btw, MeshLab handles the cases with recursive PROTO and too long names but crashes with recursive USE. Blender handles all three cases.

Good thing to know! I don’t actually use FreeCAD, I was just fuzz-testing my own VRML implementation and wondered how other software suites do (considering we live in the age of exploits). It’s just fair to give other developers every chance to improve their software, so I reported the crashes outright. You should report them to MeshLab as well; use my test files at will.

yorik

2022-03-03 13:55

administrator   ~0016542

This ticket has been migrated to GitHub as issue 5707.

Issue History

Date Modified Username Field Change
2017-06-05 01:02 Krishty New Issue
2017-06-05 01:02 Krishty File Added: corrupts memory - name too long.wrl
2017-06-05 01:02 Krishty File Added: crash - recursive PROTO.wrl
2017-06-05 01:02 Krishty File Added: crash - recursive USE.wrl
2017-06-05 01:10 Krishty File Added: corrupts memory - name too long (VRML 1).wrl
2017-06-05 01:10 Krishty File Added: crash - recursive USE (VRML 1).wrl
2017-06-05 01:10 Krishty Note Added: 0009285
2017-06-10 05:13 Kunda1 Tag Attached: VRML
2017-06-10 11:35 Kunda1 Note Added: 0009331
2017-06-11 12:16 wmayer Note Added: 0009343
2017-06-11 12:33 Krishty Note Added: 0009344
2017-06-11 12:46 wmayer Note Added: 0009345
2017-06-11 13:01 Krishty Note Added: 0009346
2017-06-11 14:49 wmayer Note Added: 0009347
2017-06-11 14:51 wmayer Status new => confirmed
2017-06-11 15:36 wmayer Note Edited: 0009347
2017-06-11 18:09 Krishty Note Added: 0009348
2021-02-06 06:31 abdullah Target Version => 0.20