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

Segfault in XXH3_64bits_reset_withSeed with x86 Visual Studio 2015 #401

Closed
ifduyue opened this issue Jun 21, 2020 · 8 comments
Closed

Segfault in XXH3_64bits_reset_withSeed with x86 Visual Studio 2015 #401

ifduyue opened this issue Jun 21, 2020 · 8 comments

Comments

@ifduyue
Copy link

ifduyue commented Jun 21, 2020

I don't know if it's a bug, I have setup a reproduce:

Command exited with code -1073741819

@Cyan4973
Copy link
Owner

Could you update the xxHash module ?
It seems to be an "old" version.
Since then, an issue related to AVX2 alignment has been fixed.
Not sure if it's the one that happens here, but just in case ...

@easyaspi314
Copy link
Contributor

easyaspi314 commented Jun 21, 2020

Are you sure it doesn't have to do with calling XXH3_64bits_digest on an uninitialized state?

@Cyan4973 We should probably switch to calloc for creating states.

@easyaspi314
Copy link
Contributor

easyaspi314 commented Jun 21, 2020

Can confirm a crash with -fsanitize=address:

#define XXH_STATIC_LINKING_ONLY
#include <stdio.h>
#include <xxhash.h>

int main(void)
{
    XXH3_state_t *state = XXH3_createState();
    printf("%llx\n", (unsigned long long)XXH3_64bits_digest(state));
    XXH3_freeState(state);
}
AddressSanitizer:DEADLYSIGNAL
=================================================================
==20253==ERROR: AddressSanitizer: SEGV on unknown address 0xfafafbfafafafaf (pc 0x0063cc51ed88 bp 0x007fd51ed3d0 sp 0x007fd51ed0e0 T0)
==20253==The signal is caused by a READ memory access.
    #0 0x63cc51ed84 in XXH3_scrambleAcc_neon /data/data/com.termux/files/home/xxhash-clean/xxh3.h:1371:35
    #1 0x63cc524644 in XXH3_consumeStripes /data/data/com.termux/files/home/xxhash-clean/xxh3.h:2009:9
    #2 0x63cc51f808 in XXH3_digest_long /data/data/com.termux/files/home/xxhash-clean/xxh3.h:2112:9
    #3 0x63cc51f1c0 in XXH3_64bits_digest /data/data/com.termux/files/home/xxhash-clean/xxh3.h:2141:9
    #4 0x63cc527adc in main /data/data/com.termux/files/home/xxcrash.c:8:22
    #5 0x6fdaeb4844 in __libc_init (/apex/com.android.runtime/lib64/bionic/libc.so+0x7d844)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /data/data/com.termux/files/home/xxhash-clean/xxh3.h:1371:35 in XXH3_scrambleAcc_neon
==20253==ABORTING

Normally it doesn't show up because most Linux implementations use mmap for malloc which zeros by default. -fsanitize=address will simulate a "dirty malloc" which shows the crash.

Undefined behavior on your side, but we could improve this a bit with calloc because it accepts a zeroed state.

@ifduyue
Copy link
Author

ifduyue commented Jun 21, 2020

@easyaspi314 you're right, it's my fault, it's a use before init error. Reset right after creating a state and everything is fine.

@ifduyue
Copy link
Author

ifduyue commented Jun 21, 2020

Sorry to bother again, thanks in advance!

Actually I'm developing python-xxhash and trying to support xxh3.
When it's v0.7.3, always error at one of test_xxh3_128_reset, test_xxh3_128_reset_more, test_xxh3_64_reset and test_xxh3_64_seed_reset, only 32-bit python3.5/3.6/3.7/3.8 on appveyor windows image.
When it's dev branch, all tests pass.

@Cyan4973 @easyaspi314 Anything special between v0.7.3 and dev?

@easyaspi314
Copy link
Contributor

easyaspi314 commented Jun 21, 2020

Well v0.7.3's state is not movable due to a pointer issue. Not sure why it only crashes on 32-bit, though. (#365, fix #366).

For the reference, it goes like this

XXH3_state_t *s1 = XXH3_createState(), *s2 = XXH3_createState;
XXH3_64bits_reset_withSeed(s1, seed);
XXH3_copyState(s2, s1);
XXH3_freeState(s1);
// s2 has a wild pointer to the internal state of s1 in v0.7.3, fixed in dev

Or simplifying the wrappers, the bug is a simple use-after-free:

typedef struct XXH3_state_s {
    unsigned char customSecret[192];
    const unsigned char *secret;
} XXH3_state_t;

XXH3_state_t *s1 = malloc(sizeof(XXH3_state_t)), *s2 = malloc(sizeof(XXH3_state_t));
s1->secret = &s1->customSecret[0];
memcpy(s2, s1, sizeof(XXH3_state_t));
assert(s2->secret == &s1->customSecret[0]);
free(s1);

For a workaround for v0.7.3, you can do this:

XXH3_copyState(s2, s1);
#if XXH_VERSION_NUMBER < 704
// v0.7.3 and earlier have a bug where states reset with a seed
// will have a wild pointer to the original state when copied,
// causing a use-after-free if the original is freed.
if (s2->secret == &s1->customSecret[0])
    s2->secret = &s2->customSecret[0];
#endif 

@ifduyue
Copy link
Author

ifduyue commented Jun 22, 2020

@easyaspi314 Thanks for your detailed reply. Very helpful.

@ifduyue ifduyue closed this as completed Jun 22, 2020
ifduyue added a commit to ifduyue/python-xxhash that referenced this issue Jun 22, 2020
ifduyue added a commit to ifduyue/python-xxhash that referenced this issue Jul 5, 2020
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