google / googletest

GoogleTest - Google Testing and Mocking Framework
https://google.github.io/googletest/
BSD 3-Clause "New" or "Revised" License
34.51k stars 10.09k forks source link

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

Closed orenazad closed 4 months ago

orenazad commented 4 months ago

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: https://github.com/google/googletest/blob/fa6de7f4382f5c8fb8b9e32eea28a2eb44966c32/googletest/src/gtest.cc#L6708

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 commented 4 months ago

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.