Skip to content

Use a temporary variable for that test - #3883

Closed
tiennou wants to merge 1 commit into
libgit2:masterfrom
tiennou:iterator-double-free-test
Closed

Use a temporary variable for that test#3883
tiennou wants to merge 1 commit into
libgit2:masterfrom
tiennou:iterator-double-free-test

Conversation

@tiennou

@tiennou tiennou commented Aug 4, 2016

Copy link
Copy Markdown
Contributor

I'll let ASAN speak for me here. That fix might be completely wrong, and it's expected to be able to run with both instances at the same time, but here I consistently get a failure when running the full test suite.

==25948==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000003260 at pc 0x0001000520c5 bp 0x7fff5fbff4a0 sp 0x7fff5fbff498
READ of size 8 at 0x614000003260 thread T0
    #0 0x1000520c4 in clear_cache cache.c:84
    #1 0x10005205f in git_cache_clear cache.c:101
    #2 0x1002527b0 in git_repository__cleanup repository.c:112
    #3 0x100252bb7 in git_repository_free repository.c:128
    #4 0x1005c72ef in test_refs_iterator__cleanup iterator.c:14
    #5 0x10037f14f in clar_run_test clar.c:232
    #6 0x10037abf1 in clar_run_suite clar.c:286
    #7 0x100379f97 in clar_test_run clar.c:420
    #8 0x1004f37a8 in main main.c:19
    #9 0x7fff956995ac in start (libdyld.dylib+0x35ac)
    #10 0x0  (+0x0)

0x614000003260 is located 32 bytes inside of 448-byte region [0x614000003240,0x614000003400)
freed by thread T0 here:
    #0 0x100e96b89 in wrap_free (libclang_rt.asan_osx_dynamic.dylib+0x48b89)
    #1 0x100253074 in git__free util.h:246
    #2 0x10025304f in git_repository_free repository.c:147
    #3 0x1005c8b59 in test_refs_iterator__concurrent_delete iterator.c:197
    #4 0x10037f074 in clar_run_test clar.c:222
    #5 0x10037abf1 in clar_run_suite clar.c:286
    #6 0x100379f97 in clar_test_run clar.c:420
    #7 0x1004f37a8 in main main.c:19
    #8 0x7fff956995ac in start (libdyld.dylib+0x35ac)
    #9 0x0  (+0x0)

previously allocated by thread T0 here:
    #0 0x100e96f27 in wrap_calloc (libclang_rt.asan_osx_dynamic.dylib+0x48f27)
    #1 0x100262ffc in git__calloc util.h:169
    #2 0x10025321a in repository_alloc repository.c:173
    #3 0x100253b8f in git_repository_open_ext repository.c:692
    #4 0x100256ab5 in git_repository_open repository.c:739
    #5 0x1005c72a7 in test_refs_iterator__initialize iterator.c:9
    #6 0x10037f001 in clar_run_test clar.c:219
    #7 0x10037abf1 in clar_run_suite clar.c:286
    #8 0x100379f97 in clar_test_run clar.c:420
    #9 0x1004f37a8 in main main.c:19
    #10 0x7fff956995ac in start (libdyld.dylib+0x35ac)
    #11 0x0  (+0x0)

SUMMARY: AddressSanitizer: heap-use-after-free cache.c:84 in clear_cache
Shadow bytes around the buggy address:
  0x1c28000005f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
  0x1c2800000600: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x1c2800000610: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2800000620: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2800000630: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x1c2800000640: fa fa fa fa fa fa fa fa fd fd fd fd[fd]fd fd fd
  0x1c2800000650: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2800000660: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2800000670: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2800000680: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x1c2800000690: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==25948==ABORTING

@pks-t

pks-t commented Aug 5, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

I don't really understand how the introduction of a new temporary variable could in any way fix the occurring issue. The global repo variable is first free'd and then set to another repository pointer and at the end of the function reset to NULL. So after all, test_refs_iterator__initialize should not cross with test_refs_iterator__cleanup and the local variable should have no impact.

So in fact I guess there's something different going on here, even though I don't know what it is. I'm unable to reproduce the issue though, as ASAN cannot even reach this segment of code due to it aborting with a false positive before. I guess you're using ASAN with clang? Which version exactly and how did you configure libgit2? Just so I am able to reproduce the issue. Thanks :)

@tiennou

tiennou commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

I honestly have no idea why it doubles-frees, which is why I'm not sure if the fix is correct. Maybe there's something that uses the address of the global var, but then the NULL reset would have taken care of that anyway...

aborting with a false positive before

The try_grow(TOOBIG) test ? I cl_skipped that one actually.

I'm using an CMake-generated Xcode project though, so from the root of the project :

mkdir build-xcode
cd build-xcode
cmake -GXcode ..

You'll end up with a .xcodeproj in there, which you can setup for ASan the usual way.

Here's my clang info (running Xcode 7.3.1 7D1014):

$ clang -v
Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.3.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@tiennou

tiennou commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Just for completeness, if you want to investigate some weirdness : I did ln -s tests/resourcesrebase/.git{ted,} (I was working on extending the status example). This caused major breakage of the test suite (asserting NULL transactions what the first thing that failed). I don't know if running against a symlinked repository is supported though...

@pks-t

pks-t commented Aug 5, 2016

Copy link
Copy Markdown
Member

It does not double-free but instead tries to use memory that has previously been free'd.

I think I've got an OSX VM stored somewhere, so I'll try to reproduce with the Apple Clang toolchain. Thanks for the specs.

And no, the TOO_BIG test is not the issue, instead it complains about a git_oid struct which is not initialized. Even though we call memset(oid, 0, sizeof(git_oid)) shortly before -- I've got no idea, but recently I've experienced more of these false positives with clang. No idea why, though.

@pks-t

pks-t commented Aug 5, 2016

Copy link
Copy Markdown
Member

One interesting thing: ASAN complains that the address that is to be accessed in clear_cache has previously been allocated in in repository_alloc repository.c:173, where we malloc the git_repository itself. So either a git_repository is ending up in the repository's object cache (highly unlikely) or the analysis done by ASAN is misleading/wrong (more likely).

@tiennou

tiennou commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

I took a closer look by doing multiple loops of "open/iterate-refs/free" because it seems the failure is intermittent — at least today I can run the refs::iterator suite without triggering ASan. I'm starting to suspect the optimizer sometimes decide that the NULL is dead code and optimize it away ? The rationale being I can pretty much reproduce that crash if I don't NULL the pointer.

This is so very weird...

@carlosmn

carlosmn commented Aug 5, 2016

Copy link
Copy Markdown
Member

If you don't set the pointer to NULL then the general cleanup function will try to free it, which will always lead to double-frees or invalid memory accesses so that doesn't really mean anything.

@tiennou

tiennou commented Aug 5, 2016

Copy link
Copy Markdown
Contributor Author

Yeah, it's obviously wrong, I was just investigating. "Forgetting" the NULL gives me the exact same crash (invalid access in the middle of a free'd git_repo, same position), which is why I find it weird. I mean, I wasn't able to run ASan on master yesterday until I made those changes, and today it doesn't care anymore ?!

@pks-t

pks-t commented Aug 29, 2016

Copy link
Copy Markdown
Member

Are you still able to reproduce the issue?

@tiennou

tiennou commented Aug 29, 2016

Copy link
Copy Markdown
Contributor Author

Haven't seen it after 2 full tests run with ASan enabled. Really weird.

I have a few changes for ASan compatibility, and I can upstream them if there's interest in those. Seems that ASan is supported by clang >= 3.1 or GCC >= 4.8.

@tiennou tiennou closed this Aug 29, 2016
@tiennou tiennou mentioned this pull request Aug 29, 2016
Closed
@tiennou
tiennou deleted the iterator-double-free-test branch September 16, 2017 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiennou @pks-t @carlosmn