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

[Kim Barrett]

On Wed, 27 Nov 2024 07:48:00 GMT, SendaoYan <syan 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;
}

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

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