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

Remove O1 from sanitizer feature flag defaults #17355

Closed

Conversation

chiragramani
Copy link
Contributor

@chiragramani chiragramani commented Jan 30, 2023

This PR removes -O1 from the current set of sanitizer related feature flags defaults.

Context and Repro

  1. Heap buffer overflow in the following code block is not caught by asan.

example.cc

#include <cstdlib>

int main(int argc, char **argv) {
  int *array = new int[100];
  array[0] = 0;
  int res = array[argc + 100];  // BOOM
  delete [] array;
  return res;
}

BUILD

cc_binary(
  name = 'example',
  srcs = ['example.cc'],
  features = ['asan'],
)

execute:

bazel run :example

Expectation:
Address sanitizer should detect and report heap buffer overflow.

But this doesn't happen in the above case. It is because of O1 being applied by default and since this is added at the last, it also overrides explicit copts passed(O0). It would be nice if the optimization level is a bit de-coupled from the default group here.

@chiragramani
Copy link
Contributor Author

cc: @brentleyjones

@fmeum
Copy link
Collaborator

fmeum commented Jan 30, 2023

@chiragramani I can't reproduce this on Linux with clang 14. What is your setup?

Given that -O0 tends to lead to much slower code, ideally we would find less invasive flags that allow you to reproduce the ASan finding in this case. This is especially relevant when combining sanitizers with fuzzing, where execution speed is key.

@brentleyjones
Copy link
Contributor

But can't you add -O1 if you need it? The main issue for us is that it forces this change on us, especially in the rules_xcodeproj case.

@fmeum
Copy link
Collaborator

fmeum commented Jan 30, 2023

@brentleyjones Sorry, I somehow thought this would add -O0 instead of -O1 - yes, I agree that the sanitizer features shouldn't affect the optimization level by themselves.

If the C++ toolchain framework supports it, we could add flags we know work well depending on the build mode, but that's a separate effort.

Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

@oquenchil Could you review?

@fmeum
Copy link
Collaborator

fmeum commented Jan 30, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 30, 2023
@fmeum
Copy link
Collaborator

fmeum commented Jan 30, 2023

@chiragramani Could you also remove the -O1 flag added in #17083 for consistency?

@chiragramani
Copy link
Contributor Author

@chiragramani Could you also remove the -O1 flag added in #17083 for consistency?

Thanks for reviewing it, updated the PR.

@ShreeM01
Copy link
Contributor

@bazel-io fork 6.1.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jan 30, 2023
@oquenchil oquenchil self-assigned this Feb 3, 2023
@oquenchil oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Feb 3, 2023
@copybara-service copybara-service bot closed this in e132653 Feb 7, 2023
ShreeM01 added a commit that referenced this pull request Feb 8, 2023
This PR removes `-O1` from the current set of sanitizer related feature flags defaults.

**Context and Repro**
1. Heap buffer overflow in the following code block is not caught by asan.

example.cc
```
#include <cstdlib>

int main(int argc, char **argv) {
  int *array = new int[100];
  array[0] = 0;
  int res = array[argc + 100];  // BOOM
  delete [] array;
  return res;
}
```
BUILD
```
cc_binary(
  name = 'example',
  srcs = ['example.cc'],
  features = ['asan'],
)
```
execute:
```
bazel run :example
```

**Expectation:**
Address sanitizer should detect and report heap buffer overflow.

But this doesn't happen in the above case. It is because of O1 being applied by default and since this is added at the last, it also overrides explicit copts passed(O0). It would be nice if the optimization level is a bit de-coupled from the default group here.

Closes #17355.

PiperOrigin-RevId: 507658773
Change-Id: I3aa4fb92a2dc271cbbedfc6f05e72a8a9b2aba09

Co-authored-by: Chirag Ramani <chirag.ramani7@gmail.com>
keith added a commit to bazelbuild/apple_support that referenced this pull request Feb 12, 2023
keith added a commit to bazelbuild/apple_support that referenced this pull request Feb 12, 2023
hvadehra pushed a commit that referenced this pull request Feb 14, 2023
This PR removes `-O1` from the current set of sanitizer related feature flags defaults.

**Context and Repro**
1. Heap buffer overflow in the following code block is not caught by asan.

example.cc
```
#include <cstdlib>

int main(int argc, char **argv) {
  int *array = new int[100];
  array[0] = 0;
  int res = array[argc + 100];  // BOOM
  delete [] array;
  return res;
}
```
BUILD
```
cc_binary(
  name = 'example',
  srcs = ['example.cc'],
  features = ['asan'],
)
```
execute:
```
bazel run :example
```

**Expectation:**
Address sanitizer should detect and report heap buffer overflow.

But this doesn't happen in the above case. It is because of O1 being applied by default and since this is added at the last, it also overrides explicit copts passed(O0). It would be nice if the optimization level is a bit de-coupled from the default group here.

Closes #17355.

PiperOrigin-RevId: 507658773
Change-Id: I3aa4fb92a2dc271cbbedfc6f05e72a8a9b2aba09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants