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

[SendaoYan]

On Wed, 27 Nov 2024 15:08:13 GMT, SendaoYan <syan at openjdk.org> wrote:

>> 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 check this change can avoid gcc warning or not with -fsanitize=undefined -fsanitize=address.

The four strlen calculation has been combine to one. Thanks.

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

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