RFR: 8345043: [ASAN] methodMatcher.cpp report reading from a region of size 0 [-Werror=stringop-overread]

[SendaoYan]

On Wed, 27 Nov 2024 12:17:56 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Hi all,
>> The file src/hotspot/share/compiler/methodMatcher.cpp report compile warning by gcc14 with -fsanitize=undefined -fsanitize=address `‘size_t strlen(const char*)’ reading 1 or more bytes from a region of size 0 [-Werror=stringop-overread]`. I think it's false positive, the `if (name[0] == '*')` and `if (strlen(name) == 1)` judgement has make sure that the length of name greater or equal to 2, but the static analyze at compile time is unable to identify that.
>> So this PR add PRAGMA_DISABLE_GCC_WARNING("-Wstringop-overread") for the line to disable the false positive gcc warning.  Risk is low.
>
> src/hotspot/share/compiler/methodMatcher.cpp line 231:
> 
>> 229:     // This code can incorrectly cause a "stringop-overread" warning with gcc
>> 230:      memmove(name, name + 1, strlen(name + 1) + 1);
>> 231: PRAGMA_DIAG_POP
> 
> This function has 4 `strlen(name)` expressions.  It could be changed to have
> only one, and then update the value as it proceeds. I wonder if changing it
> that way might dodge whatever is confusing ASAN, as well as (arguably) making
> the function a bit clearer.  I've no idea how important (or not) reducing the
> number of strlen calls might be for performance.
> 
> So something like this (not tested):
> 
> static MethodMatcher::Mode check_mode(char name[], const char*& error_msg) {
>   int match = MethodMatcher::Exact;
>   size_t len = strlen(name);
>   if (name[0] == '*') {
>     if (len == 1) {
>       return MethodMatcher::Any;
>     }
>     match |= MethodMatcher::Suffix;
>     memmove(name, name + 1, len); // Include terminating nul in move.
>     len -= 1;
>   }
> 
>   if (len > 0 && name[len - 1] == '*') {
>     match |= MethodMatcher::Prefix;
>     name[--len] = '\0';
>   }
> 
>   if (len == 0) {
>     error_msg = "** Not a valid pattern";
>     return MethodMatcher::Any;
>   }
> 
>   if (strstr(name, "*") != nullptr) {
>     error_msg = " Embedded * not allowed";
>     return MethodMatcher::Unknown;
>   }
>   return (MethodMatcher::Mode)match;
> }

Thanks your advice. I think the performance impact maybe can be ignored, since this function used for parser `-XX:CompileCommand`. I will try this change can avoid gcc warning or not with -fsanitize=undefined -fsanitize=address.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22406#discussion_r1860831488