#1029: resolve issue with bad memory access in ranged for loop found on Mac OS (clang compiler) #1033
Conversation
… loop found on Mac OS (clang compiler)
| for (auto split : *splits) | ||
| for (size_t idx = 0; idx < splits->size(); ++idx) | ||
| { | ||
| if (!split->pts && split->splits && | ||
| auto split = (*splits)[idx]; | ||
| if (split && !split->pts && split->splits && |
There was a problem hiding this comment.
While this fixes the crash, I fear it might be really hiding something more concerning, but I'm not familiar with the codebase:
The call to CheckBounds() below may modify the splits vector passed to this function.
Modifying any vector for which iterators exist will invalidate said iterators, which is why the range-based for loop breaks. Using an index with a null check (which you did here) might fix it, but only if the modifications are limited to appending items.
I guess my main question is: Is this indentional? It kind of looks like what's happening is that CheckSplitOwner() is a recursive function which sometimes is called multiple times on the same object.
There was a problem hiding this comment.
The call to CheckBounds() below may modify the splits vector passed to this function.
Arrh, indeed! I hadn't spotted that (and my function naming didn't help because it does so much more than just check bounds 😡). CheckBounds calls numerous nested functions including FixSelfIntersects which in turn may call DoSplitOp which could append to the splits std::vector<OutRec>* - potentially invalidating the splits iterator 😱.
I'm yet to decide how best to address this (and indexing as suggested by Nicolas I think should work well, though I welcome other suggestions 😁). And it's likely that additional OutRec structs created during the current for loop inside CheckSplitOwner will also need parsing.
Edit: I really do hate the PolyPath struct because it adds enormous complexity to the library. However, I do understand how useful (and important) it is for many library users.
Edit2: And I can't see how split could be a null pointer inside this loop.
3 participants
I believe the issue is related to a memory access after the underlying std::vector has been altered.