Skip to content

Conversation

@axic
Copy link
Contributor

@axic axic commented Nov 28, 2019

Attempt to solve #5869, but couldn't find any reproducible test case with any compiler version. Any ideas @bshastry? The test case in the fuzzer issue is not reproducing it.

Copy link
Collaborator

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

If all CI runs pass, I don't see a reason not to do this... Unfortunately json-cpp doesn't seem to have proper Changelogs, so we can't really check, if there's something problematic in there...

Copy link
Contributor

@bshastry bshastry left a comment

Choose a reason for hiding this comment

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

Looks good to me if tests pass. I checked locally that the oss fuzz issue is no longer reproducible with this PR (jsoncpp 1.9.2) but was reproducible for jsoncpp 1.8.4 that we are currently depending on.

@bshastry
Copy link
Contributor

The test case in the fuzzer issue is not reproducing it.

I could reproduce the test failure with clang-6, UBSan enabled, jsoncpp v1.8.4, latest solidity develop commit but not with this PR.

@axic
Copy link
Contributor Author

axic commented Nov 29, 2019

The test case in the fuzzer issue is not reproducing it.

I could reproduce the test failure with clang-6, UBSan enabled, jsoncpp v1.8.4, latest solidity develop commit but not with this PR.

Hm, so no way to add a test case into our repo which would reproduce this without ubsan?

@bshastry
Copy link
Contributor

The test case in the fuzzer issue is not reproducing it.
I could reproduce the test failure with clang-6, UBSan enabled, jsoncpp v1.8.4, latest solidity develop commit but not with this PR.

Hm, so no way to add a test case into our repo which would reproduce this without ubsan?

Sadly, yes. Unless we add a UBSan build and test CIs. I feel adding a UBSan may be an overkill for this though because as far as I remember this is the only bug that was found (in a dependency and not solidity) by UBSan.

@axic
Copy link
Contributor Author

axic commented Nov 29, 2019

Is it with the test case from the issue? I can't fathom how that could lead into this. Is it due to some gas estimation field?

@axic axic marked this pull request as ready for review November 29, 2019 12:23
@leonardoalt leonardoalt merged commit c6c5a6f into develop Nov 29, 2019
@leonardoalt leonardoalt deleted the jsoncpp branch November 29, 2019 12:25
resonant-riches added a commit to ChorusOne/azimuth-cli that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants