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

[Bug]: ASAN heap-buffer-overflow in ParseGoogleTestFlagsOnlyImpl() in gtest.cc #4532

Closed
orenazad opened this issue May 8, 2024 · 1 comment
Assignees

Comments

@orenazad
Copy link

orenazad commented May 8, 2024

Describe the issue

When running googletest with Address Sanitizer (via Xcode 15.2.0 for ARM) there is a buffer-overflow:

==15724==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x00010c1f9340 at pc 0x000103282fe4 bp 0x00016d58fed0 sp 0x00016d58fec8
READ of size 8 at 0x00010c1f9340 thread T0
    #0 0x103282fe0 in void testing::internal::ParseGoogleTestFlagsOnlyImpl<char>(int*, char**) gtest.cc:6599
    #1 0x103283e98 in void testing::internal::InitGoogleTestImpl<char>(int*, char**) gtest.cc:6685

The faulty code is at line 6702 in gtest.cc (at time of writing):
argv[j] = argv[j + 1];

    if (remove_flag) {
      // Shift the remainder of the argv list left by one.  Note
      // that argv has (*argc + 1) elements, the last one always being
      // NULL.  The following loop moves the trailing NULL element as
      // well.
      for (int j = i; j != *argc; j++) {
        argv[j] = argv[j + 1];
      }

      // Decrements the argument count.
      (*argc)--;

      // We also need to decrement the iterator as we just removed
      // an element.
      i--;
    }

While the comment is correct and the logic to move the trailing NULL element works in practice- this throws a buffer overflow error for ASAN as it is technically moving past the last index in the array. Although it may not cause any issues, this should be corrected as a pre-caution (and to ensure googletest can run with ASAN).

The change to fix this can be relatively simple. Here is one I tried locally that worked:

    if (remove_flag) {
      // Shift the remainder of the argv list left by one.
      for (int j = i; j < *argc - 1; j++) {
        argv[j] = argv[j + 1];
      }

      // Manually set the last element to NULL.
      argv[*argc - 1] = NULL;

      // Decrements the argument count.
      (*argc)--;

      // We also need to decrement the iterator as we just removed
      // an element.
      i--;
    }

Steps to reproduce the problem

  1. Enable Address Sanitizer in XCode 15.2
  2. Build googletest with Address Sanitizer
  3. Run googletest

What version of GoogleTest are you using?

git rev-parse HEAD 
8ad443bf120be22a4b7479790d408c7f7036a0b7

The code remains the same on main at time of writing:

argv[j] = argv[j + 1];

What operating system and version are you using?

MacOS Ventura 13.6

What compiler and version are you using?

Apple Clang / XCode 15.2.0

What build system are you using?

N/A

Additional context

No response

@tobbi
Copy link
Contributor

tobbi commented May 16, 2024

I guess the best way for you would be to create a pull request. But be prepared that Google employees don't really check and/or merge pull requests that often.

@derekmauro derekmauro self-assigned this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants