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

fixed stack smashing due to wrong size of struct stat on the stack #4631

Merged
merged 1 commit into from
Apr 20, 2018
Merged

fixed stack smashing due to wrong size of struct stat on the stack #4631

merged 1 commit into from
Apr 20, 2018

Conversation

andreasbaumann
Copy link
Contributor

on 32-bit systems with 64-bit file descriptor offsets enabled
(added -D_FILE_OFFSET_BITS=64 when compiling the test suite)

on 32-bit systems with 64-bit file descriptor offsets enabled
(added -D_FILE_OFFSET_BITS=64 when compiling the test suite)
@andreasbaumann
Copy link
Contributor Author

This happens only on 32-bit Intel systems with stack smash protection enabled.

@andreasbaumann
Copy link
Contributor Author

As one test case shows:

shell> ./libgit2_clar -v -s"core::copy"

*** stack smashing detected ***: <unknown> terminated
==6816==    at 0x4C3C112: raise (in /usr/lib/libc-2.26.so)
==6816==    by 0x4C3D891: abort (in /usr/lib/libc-2.26.so)
==6816==    by 0x4C7ECFA: __libc_message (in /usr/lib/libc-2.26.so)
==6816==    by 0x4D1735A: __fortify_fail_abort (in /usr/lib/libc-2.26.so)
==6816==    by 0x4D17309: __stack_chk_fail (in /usr/lib/libc-2.26.so)
==6816==    by 0x3B5E22: __stack_chk_fail_local (in /build/libgit2/src/libgit2-
==6816==    by 0x179522: test_core_copy__file (in /build/libgit2/src/libgit2-0.
==6816==    by 0x129F60: clar_run_test.isra.5 (in /build/libgit2/src/libgit2-0.
==6816==    by 0x12A315: clar_test_run (in /build/libgit2/src/libgit2-0.27.0/li
==6816==    by 0x127847: main (in /build/libgit2/src/libgit2-0.27.0/libgit2_cla

Breakpoint 1, test_core_copy__file () at /media/sd/libgit2/tests/core/copy.c:7

@ethomson
Copy link
Member

What happens? Where? Can you explain a little bit more about the problem that you're solving here?

Are we assuming that we have 64 bit types in struct stat? If so, wouldn't this be a problem on 32 bit systems that have no 64 bit interface offered with _FILE_OFFSET_BITS or similar?

@ethomson
Copy link
Member

Thanks for the stacktrace. That's helpful. This is unexpected... I would have thought that struct stat and lstat should both honor either _FILE_OFFSET_BITS=64 or _FILE_OFFSET_BITS=32... And although I can understand why you would want _FILE_OFFSET_BITS=64, I feel like we shouldn't require that. It feels like there's a root cause that I'm not yet seeing. Are we incorrectly assuming a 64 bit type somewhere else when we should be detecting the size?

@andreasbaumann
Copy link
Contributor Author

andreasbaumann commented Apr 19, 2018

Sorry, I should maybe have reported it as a bug and not as a pull-request. :-)

As I understand it, the size of struct stat depends on the value of DFILE_OFFSET_BITS.
As in src/CMakeLists.txt there is a -DFilE_OFFSET_BITS=64, I just added it also to
tests/CMakeLists.txt, so that they are both the same.

@ethomson
Copy link
Member

Aha! Thanks, that's the missing link for me.

Yeah, when we split up the CMakeLists.txt into one for the library and one for tests, these got out of whack. Thanks, this makes sense.

@pks-t pks-t added the backport label Apr 20, 2018
@pks-t pks-t merged commit 709e099 into libgit2:master Apr 20, 2018
@pks-t
Copy link
Member

pks-t commented Apr 20, 2018

Thanks for your fix!

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

Successfully merging this pull request may close these issues.

None yet

3 participants