libc: suppress ASan warnings for overflow test cases#468
libc: suppress ASan warnings for overflow test cases#468
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where AddressSanitizer (ASan) would report errors during tests specifically designed to check for memory allocation overflow conditions. By conditionally redirecting these overflow tests to use the underlying Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces conditional compilation to suppress AddressSanitizer warnings for memory allocation overflow tests, which is a reasonable approach. My review includes a suggestion to refactor the preprocessor macros for better consistency and maintainability. I've also identified and suggested a fix for a logical bug in one of the overflow tests where an incorrect variable was being used, causing the test to not perform its intended check.
libc/stdlib/stdlib_alloc.c
Outdated
| TEST_ASSERT_NOT_NULL(ptr); | ||
| TEST_ASSERT_EACH_EQUAL_INT(0, ptr1, BLOCK_SIZE / sizeof(int)); | ||
| TEST_ASSERT_NULL(realloc(ptr, SIZE_MAX)); | ||
| TEST_ASSERT_NULL(TEST_REALLOC(ptr, SIZE_MAX)); |
There was a problem hiding this comment.
The intent of this test block seems to be to check for overflow in realloc when using a pointer originally allocated by calloc (ptr1). However, realloc is called with ptr, which was allocated by malloc and already tested in the previous block. You should use ptr1 here to test the calloc path.
TEST_ASSERT_NULL(TEST_REALLOC(ptr1, SIZE_MAX));There was a problem hiding this comment.
Huh, that does seem like a bug in the original test case
6df98c1 to
3e406da
Compare
JIRA: CI-657
532c73c to
7d38dd8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces macros to bypass ASan instrumentation for memory allocation functions in overflow tests, which is a valid approach to handle the intentional overflow checks. The changes correctly apply these macros in the test cases. Additionally, a bug in the realloc_overflow test where an incorrect pointer was used has been fixed. My only suggestion is to refactor the preprocessor logic for defining the new macros to improve its readability and maintainability.
libc/stdlib/stdlib_alloc.c
Outdated
| return "allocator_may_return_null=1"; | ||
| } | ||
|
|
||
| #if defined(__linux__) && defined(__SANITIZE_ADDRESS__) |
There was a problem hiding this comment.
For sure we should leave a comment why we are doing that - it's not sth typical in this code.
There was a problem hiding this comment.
Also, mention that it's valid only for glibc
libc/stdlib/stdlib_alloc.c
Outdated
| } | ||
|
|
||
| #if defined(__linux__) && defined(__SANITIZE_ADDRESS__) | ||
| #define __linux_with_ASan__ |
There was a problem hiding this comment.
Identifiers beginning with __ (double underscore) are reserved for the implementation (compiler and standard library) by the C standard. Defining such a macro in user code is undefined behaviour and risks silently conflicting with compiler/system headers in future toolchain versions.
| #define __linux_with_ASan__ | |
| #define ALLOC_TEST_LINUX_ASAN |
There was a problem hiding this comment.
- as gemini pointed out - no need for extra define.
- is this needed to be included in
#pragma diagnostic? - as you-re embeddid multiple ifdefs (you're inside
#ifndef __phoenix__here - please add comments to#endifwith the scope name you're leaving)
There was a problem hiding this comment.
I decided to pursue the approach without an extra define. All the rest is done/ fixed
7d38dd8 to
29baeea
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to bypass AddressSanitizer interception for memory allocation functions in overflow tests. This is achieved by using macros that call internal glibc functions directly when ASan is active on Linux. The changes correctly apply these macros to the overflow tests for malloc, calloc, and realloc. The pull request also includes fixes for existing bugs within the realloc_overflow test case. I have one suggestion to make the conditional compilation directive more precise to avoid potential issues on non-glibc Linux distributions.
29baeea to
f4ff319
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces macros to bypass AddressSanitizer's interception of memory allocation functions, allowing overflow test cases to execute as intended. The changes are confined to the allocation tests and correctly use glibc-specific internal functions (__libc_malloc, etc.) under the appropriate preprocessor conditions. Additionally, a pre-existing bug in the realloc_overflow test case, where a wrong pointer was being used, has been corrected. The implementation is sound and achieves its goal.
JIRA: CI-657
Description
Fixes: phoenix-rtos/phoenix-rtos-project#1547
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment