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.

Sign up for GitHub

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

Jump to bottom

ASAN fixes #1375

Merged
merged 2 commits into from
Feb 13, 2023
Merged

ASAN fixes #1375

merged 2 commits into from
Feb 13, 2023

Conversation

rsmmr
Copy link
Member

@rsmmr rsmmr commented Feb 11, 2023

  • Skip clang-specific ASAN flags with other compilers.
  • Fix ASAN false positive with GCC.

@rsmmr
Skip clang-specific ASAN flags with other compilers.
9823e9f
@rsmmr
Fix ASAN false positive with GCC.
17ee047 
GCC reported false positives during stack switching for strings that
were created for debug logging. This moves the affected string into a
pre-allocated global constant.

Closes #1310.
@rsmmr rsmmr linked an issue Feb 13, 2023 that may be closed by this pull request
Zeek ASAN build with builtin-spicy/stack-use-after-scope: __fiber_switch_trampoline #1310
Closed
@rsmmr
Copy link
Member Author

rsmmr commented Feb 13, 2023

@awelzel see if this fixes #1310 for you

@rsmmr rsmmr marked this pull request as ready for review February 13, 2023 07:01
bbannier
bbannier approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@bbannier bbannier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This seems very subtle and a very specific workaround (we e.g., do not remove all allocations on these debug functions, only a few), so I'd really be curious if @awelzel can confirm the fix. Let's hope we do not break this again.

On merge this should be backported to release/1.5 and release/1.6.

@awelzel
Copy link
Contributor

awelzel commented Feb 13, 2023 •
edited
Loading

@awelzel see if this fixes #1310 for you

It does, thanks. The syslog and finger tests work without ASAN complaining.

Digression: I could now also use that Zeek build for analyzer development. However, I just observed that when using an ASAN/Debug Zeek install, compile times for a small analyzer go from ~50seconds (Release, no ASAN build) to 1min 40 seconds. So, probably need to keep a release build for that around. Neither 1min 40sec nor 50sec is pleasant, but I take the faster one.

@rsmmr
Copy link
Member Author

rsmmr commented Feb 13, 2023

This seems very subtle and a very specific workaround (we e.g., do not remove all allocations on these debug functions, only a few)

My best guess is that we're hitting undefined behavior here, where the compiler just orders stuff differently. If that's the case, I'm not sure there's really a way to use those ASAN API functions for reporting stack switching robustly.

@rsmmr
Copy link
Member Author

rsmmr commented Feb 13, 2023

And to be clear, I do think this is a false positive, I don't see anything we were doing wrong.

@rsmmr rsmmr self-assigned this Feb 13, 2023
@rsmmr rsmmr merged commit 1b12715 into main Feb 13, 2023
@rsmmr rsmmr deleted the topic/robin/gh-1310-asan-fixes branch February 13, 2023 14:16
@bbannier
Copy link
Member

Also backported to release/1.5 and release/1.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awelzel awelzel awelzel approved these changes

@bbannier bbannier bbannier approved these changes

Assignees

@rsmmr rsmmr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Zeek ASAN build with builtin-spicy/stack-use-after-scope: __fiber_switch_trampoline
3 participants
@rsmmr @awelzel @bbannier