Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

fix issue 18300: range error demangling long symbol #2062

Merged
merged 3 commits into from Feb 28, 2018

Conversation

rainers
Copy link
Member

@rainers rainers commented Jan 25, 2018

The range error occurs if the demangler happens to hit the length of the buffer (which is a multiple of 4000 by default) with the chunk it tries to write into the buffer.

I didn't add the test case to the unittests because it's a 17k string which seems excessive.

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 25, 2018

Thanks for your pull request, @rainers!

Bugzilla references

Auto-close Bugzilla Severity Description
18208 major demangle RangeError@src/core/demangle.d(230)
18300 normal core.demangle demangling of really long symbol fails

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jan 25, 2018
@Ingrater
Copy link
Contributor

That was quick. Almost like you have seen it coming ;-). Thanks for the fix.

@wilzbach
Copy link
Member

@rainers It would be great if you could add a test for it. D is trying to get more and more stable and every bug fixed shouldn't appear again. Thanks!

@rainers
Copy link
Member Author

rainers commented Jan 28, 2018

Adding the symbol from the test case to the unittests in core.demangle would increase the file size by about 30k, probably with a measurable impact on every import of core.demangle.
Maybe we should move the demangler tests into a separate file in the test folder...

@wilzbach
Copy link
Member

Maybe we should move the demangler tests into a separate file in the test folder...

Oh, I thought that's already the case.

@rainers
Copy link
Member Author

rainers commented Jan 28, 2018

I added a unittest that triggers the problem with small symbols, too, by passing a small initial buffer (first range error with buffer size 4).

auto ds = demangle(s, buf);
assert(ds == "pure nothrow @safe char[] core.demangle.demangle(const(char)[], char[])");
}
}
Copy link
Member

@wilzbach wilzbach Jan 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


edit: added as unittest below

@rainers
Copy link
Member Author

rainers commented Feb 27, 2018

changed commit message to also reference https://issues.dlang.org/show_bug.cgi?id=18208, as 18300 turnedout to be a duplicate.

@timotheecour
Copy link
Contributor

timotheecour commented Feb 27, 2018

@rainers
Copy link
Member Author

rainers commented Feb 27, 2018

FWIW #2116 uses a single small-sh symbol to trigger the bug (and fix it), but 0..10_000 seems more systematic OTOH.

This PR also contains a test for a small symbol for a variety of initial buffer length.

as mentioned in both of my PRs, we can trigger bugs more economically by passing in a small dst instead of the built-in dst

Sounds good, especially in the case of the other "remove" bug. In issue 18300 the limit has to be hit exactly, which does not seem very probable to begin with (there are thousands of large symbols in the phobos unittests but none triggered the error).

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I added the unittest without changing the buffer size below and I think this rested in the PR queue way too long

@wilzbach wilzbach changed the base branch from master to stable February 27, 2018 23:34
@wilzbach
Copy link
Member

In the interest of getting this in before 2.079, I rebased to stable and due to our CI crisis, I'm going to merge this now manually.

@wilzbach wilzbach merged commit 93cfe87 into dlang:stable Feb 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
5 participants