Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[infra] Add UBSAN_OPTIONS=\"silence_unsigned_overflow=1\" to base-builder image (#1715). #1717

Merged
merged 1 commit into from Aug 15, 2018

Conversation

Dor1s
Copy link
Contributor

@Dor1s Dor1s commented Aug 15, 2018

@Dor1s Dor1s requested review from oliverchang and kcc August 15, 2018 22:16
Copy link
Contributor

@kcc kcc left a comment

Choose a reason for hiding this comment

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

LGTM
I see at least one risk: someone may want to use their own UBSAN_OPTIONS during the build and will overwrite our value. Not sure if this will actually happen.

@Dor1s
Copy link
Contributor Author

Dor1s commented Aug 15, 2018

Fair point. I do hope that they would do UBSAN_OPTIONS="$UBSAN_OPTIONS:something", but let's see :)

@Dor1s Dor1s merged commit 137bc2c into master Aug 15, 2018
@Dor1s Dor1s deleted the silence branch August 30, 2018 03:59
tmatth pushed a commit to tmatth/oss-fuzz that referenced this pull request Oct 22, 2018
@LebedevRI
Copy link
Contributor

This crippled the two projects that did care about these unsigned overflows :/
https://reviews.llvm.org/D54771

bogner pushed a commit to bogner/llvm-zipper-prototype that referenced this pull request Nov 21, 2018
…l* unsigned overflows

Summary:
D48660 / rL335762 added a `silence_unsigned_overflow` env flag for [[ google/oss-fuzz#1717 | oss-fuzz needs ]],
that allows to silence the reports from unsigned overflows.
It makes sense, it is there because `-fsanitize=integer` sanitizer is not enabled on oss-fuzz,
so this allows to still use it as an interestingness signal, without getting the actual reports.

However there is a slight problem here.
All types of unsigned overflows are ignored.
Even if `-fno-sanitize-recover=unsigned` was used (which means the program will die after the report)
there will still be no report, the program will just silently die.

At the moment there are just two projects on oss-fuzz that care:
* [[ https://github.com/google/oss-fuzz/blob/8eeffa627f937040aaf8ba1b7d93f43f77d74fb9/projects/llvm_libcxx/build.sh#L18-L20 | libc++ ]]
* [[ https://github.com/google/oss-fuzz/blob/8eeffa627f937040aaf8ba1b7d93f43f77d74fb9/projects/librawspeed/build.sh | RawSpeed ]] (me)

I suppose this could be overridden there ^, but i really don't think this is intended behavior in any case..

Reviewers: kcc, Dor1s, #sanitizers, filcab, vsk, kubamracek

Reviewed By: Dor1s

Subscribers: dberris, mclow.lists, llvm-commits

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D54771

git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@347415 91177308-0d34-0410-b5e6-96231b3b80d8
llvm-git-migration pushed a commit to llvm-git-prototype/llvm that referenced this pull request Nov 21, 2018
…l* unsigned overflows

Summary:
D48660 / rL335762 added a `silence_unsigned_overflow` env flag for [[ google/oss-fuzz#1717 | oss-fuzz needs ]],
that allows to silence the reports from unsigned overflows.
It makes sense, it is there because `-fsanitize=integer` sanitizer is not enabled on oss-fuzz,
so this allows to still use it as an interestingness signal, without getting the actual reports.

However there is a slight problem here.
All types of unsigned overflows are ignored.
Even if `-fno-sanitize-recover=unsigned` was used (which means the program will die after the report)
there will still be no report, the program will just silently die.

At the moment there are just two projects on oss-fuzz that care:
* [[ https://github.com/google/oss-fuzz/blob/8eeffa627f937040aaf8ba1b7d93f43f77d74fb9/projects/llvm_libcxx/build.sh#L18-L20 | libc++ ]]
* [[ https://github.com/google/oss-fuzz/blob/8eeffa627f937040aaf8ba1b7d93f43f77d74fb9/projects/librawspeed/build.sh | RawSpeed ]] (me)

I suppose this could be overridden there ^, but i really don't think this is intended behavior in any case..

Reviewers: kcc, Dor1s, #sanitizers, filcab, vsk, kubamracek

Reviewed By: Dor1s

Subscribers: dberris, mclow.lists, llvm-commits

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D54771

llvm-svn=347415
@LebedevRI
Copy link
Contributor

done ^. looking forward seeing all the shiny new issues that creeped into code since then :)

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.

None yet

3 participants