Repository navigation
Conversation
Parsing recursed once per nested sequence with no depth limit, so a conformant file with thousands of nesting levels exhausted the native stack and killed the process. Track depth per thread and throw gdcm::Exception beyond a configurable limit (default 1000).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parsing a data set recurses once per nested sequence, and PS3.5 sets no nesting limit, so a conformant file can exhaust the native stack and kill the process before any error is reported.
248 frames of that cycle on a file with 6,100 nested sequences. The same file kills the released 3.2.6 wheel with
0xC00000FD(STATUS_STACK_OVERFLOW), andpython-gdcmusers lose the interpreter.The bound, stated honestly
I am not claiming real files nest 6,000 deep — they do not. The limit is a function of the stack, not of the depth:
So if the only configuration is a default 8 MB stack, you need a fairly silly file to hit this. What makes it worth fixing is that 512 KB is not exotic: threads created with a reduced stack, musl's small thread default, and sanitizer builds all sit in that range, and there 100 levels is enough — a depth real data can reach through nested functional groups.
DCMTK hit the same defect and fixed it by tracking depth and refusing beyond a configurable limit; its master handles these files cleanly. This does the same for GDCM.
The change
ParseDepthGuardcounts nesting depth per thread (athread_local, so concurrent parses do not interfere and the hot path takes no lock) and throwsgdcm::Exceptiononce the limit is reached. An instance is created at the top ofItem::Read— the single point every nesting level passes through — and the limit defaults to 1000, withParseDepthGuard::SetLimit()available for applications that carry deeper data.Nothing else changes: the guard only fires where the parser would otherwise have recursed until the stack ran out.
Verified
0xC00000FD, process diesAddressSanitizer: stack-overflowThe exception message ("Nesting depth limit exceeded while parsing data set") is swallowed by
gdcmdump's catch-all, which is why the tool reports only "Failed to read" — a caller that catchesgdcm::Exceptionsees the reason.The repository description asks for pull requests rather than issues, so the report lives here.