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

[DeviceAsan] Enlarge nullpointer redzone to better detect problem #2243

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yingcong-wu
Copy link
Contributor

Also, print out the triggering address in the error report.

@yingcong-wu yingcong-wu requested a review from a team as a code owner October 25, 2024 06:43
@github-actions github-actions bot added loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification labels Oct 25, 2024
@yingcong-wu yingcong-wu marked this pull request as draft October 25, 2024 07:02
@yingcong-wu yingcong-wu marked this pull request as ready for review October 25, 2024 07:11
@yingcong-wu yingcong-wu changed the title [DeviceAsan] Enlarge nullpointer redzone to better detect problem. [DeviceAsan] Enlarge nullpointer redzone to better detect problem Oct 25, 2024
@@ -16,6 +16,8 @@
#include "common.hpp"
#include <unordered_set>

#define NULLPTR_REDZONE_SIZE (4096)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment about why choose this size

Copy link
Contributor

@AllanZyne AllanZyne Oct 25, 2024

Choose a reason for hiding this comment

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

this macro needn't be in header file, and I suggest using constexpr to define this constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I choose this size becuase it is a common page value, and I don't have any other good reasons for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can query the size of page by GetVirtualMemGranularity.

Copy link
Contributor Author

@yingcong-wu yingcong-wu Oct 25, 2024

Choose a reason for hiding this comment

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

I don't think that is necessary. Even when we have the page size, how many pages should we use? I think some simple value is good enough for now.
Also, this value don't have to align with any page size, that is not such requirement for it.

Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug sanitizer Sanitizer layer issues/changes/specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants