| Message ID | 20241111090534.66439-2-frolov@swemel.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: fix heap-use-after-free | expand |
On Mon, 11 Nov 2024 at 14:37, Dmitry Frolov <frolov@swemel.ru> wrote: > "int main(int argc, char **argv, char** envp)" is non-standart > Microsoft`s extention of the C language and it`s not portable. > In my particular case (Debian 13, clang-16) this raises wild-pointer > dereference with ASAN message "heap-use-after-free". ... > qos_printf("ENVIRONMENT VARIABLES: {\n"); > - for (char **env = envp; *env != 0; env++) { > + for (char **env = environ; *env != 0; env++) { > qos_printf("\t%s\n", *env); > } * For heap-use-after-free, there needs to be a free(*env) call somewhere. In the 'tests/qtest/qos-test.c' file, I couldn't see environment variables being free'd anywhere. Above loop is only printing them. Following small test.c did not reproduce the 'heap-use-after-free' error. === #include <stdio.h> int main(int argc, char *argv[], char **envp) { int n = 0; for (char **p = envp; *p != 0; p++) { printf("environ[%d]: %s\n", n++, *p); } return 0; } $ cc -xc -o test test.c -lasan === * While the patch is okay, it is not clear why it fixes the wild-pointer dereference and "heap-use-after-free" errors. Thank you. --- - Prasad
On 11.11.2024 14:47, Prasad Pandit wrote: > On Mon, 11 Nov 2024 at 14:37, Dmitry Frolov <frolov@swemel.ru> wrote: >> "int main(int argc, char **argv, char** envp)" is non-standart >> Microsoft`s extention of the C language and it`s not portable. >> In my particular case (Debian 13, clang-16) this raises wild-pointer >> dereference with ASAN message "heap-use-after-free". > ... >> qos_printf("ENVIRONMENT VARIABLES: {\n"); >> - for (char **env = envp; *env != 0; env++) { >> + for (char **env = environ; *env != 0; env++) { >> qos_printf("\t%s\n", *env); >> } > * For heap-use-after-free, there needs to be a free(*env) call > somewhere. In the 'tests/qtest/qos-test.c' file, I couldn't see > environment variables being free'd anywhere. Above loop is only > printing them. Above loop dereferences the pointer env, which is pointing to the memory area, which is not allowed to read. > Following small test.c did not reproduce the > 'heap-use-after-free' error. > === > #include <stdio.h> > int > main(int argc, char *argv[], char **envp) > { > int n = 0; > for (char **p = envp; *p != 0; p++) { > printf("environ[%d]: %s\n", n++, *p); > } > return 0; > } > $ cc -xc -o test test.c -lasan > === > > * While the patch is okay, it is not clear why it fixes the > wild-pointer dereference and "heap-use-after-free" errors. > > Thank you. > --- > - Prasad > This example will work everywhere, where env pointer is supported by OS/compiler/etc... Nevertheless, I am pointing on 2 facts: 1. "env" is Microsoft`s extension, not a standard 2. There is exact example, where standards violation raises undefined behavior: debian13/clang16
On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов <frolov@swemel.ru> wrote: > Above loop dereferences the pointer env, which is pointing to > the memory area, which is not allowed to read. * Not allowed to read environment variables? Is it because Debian/clang does not support the '**envp' parameter? Is '**envp' set to NULL on Debian? If '**envp' is not supported, then the compiler should throw an error at build time, no? > I am pointing on 2 facts: > 1. "env" is Microsoft`s extension, not a standard > 2. There is exact example, where standards violation raises > undefined behavior: debian13/clang16 > * If this is about Debian not supporting '**envp' parameter, then it'll help if the commit message says "...Debian does not support this non-standard extension and crashes QEMU". The asan error makes it sound like the patch fixes the use-after-free issue. What happens if the -lasan is not used? Does it still crash QEMUt? Thank you. --- - Prasad
On 11.11.2024 15:51, Prasad Pandit wrote: > On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов<frolov@swemel.ru> wrote: >> Above loop dereferences the pointer env, which is pointing to >> the memory area, which is not allowed to read. > * Not allowed to read environment variables? Is it because > Debian/clang does not support the '**envp' parameter? Is '**envp' set > to NULL on Debian? If '**envp' is not supported, then the compiler > should throw an error at build time, no? Not allowed to read the exact memory area, because it is marked as freed. >> I am pointing on 2 facts: >> 1. "env" is Microsoft`s extension, not a standard >> 2. There is exact example, where standards violation raises >> undefined behavior: debian13/clang16 >> > * If this is about Debian not supporting '**envp' parameter, then > it'll help if the commit message says "...Debian does not support this > non-standard extension and crashes QEMU". Since this is UB, it does not matter, if a crash happens or not. ASAN just helps to highlight the hidden problem. > The asan error makes it > sound like the patch fixes the use-after-free issue. I didn`t want to confuse anybody, but this is exactly, what is actually happening (see log below). > What happens if > the -lasan is not used? Does it still crash QEMUt? > > Thank you. > --- > - Prasad > When saintizers are disabled, qos-test passes successfully. qos-test fails when qemu is built with enabled sanitizers (--enable-asan --enable-ubsan) ==879133==ERROR: AddressSanitizer: heap-use-after-free on address 0x514000000040 at pc 0x55eae79b407c bp 0x7ffd028715d0 sp 0x7ffd028715c8 READ of size 8 at 0x514000000040 thread T0 #0 0x55eae79b407b in main /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33 #1 0x7f9011760c89 (/lib/x86_64-linux-gnu/libc.so.6+0x27c89) (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) #2 0x7f9011760d44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x27d44) (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) #3 0x55eae77a5c60 in _start (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x209c60) (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) 0x514000000040 is located 0 bytes inside of 416-byte region [0x514000000040,0x5140000001e0) freed by thread T0 here: #0 0x55eae7840ce9 in __interceptor_realloc (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) #1 0x7f901177b596 (/lib/x86_64-linux-gnu/libc.so.6+0x42596) (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) previously allocated by thread T0 here: #0 0x55eae7840ce9 in __interceptor_realloc (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) #1 0x7f901177b596 (/lib/x86_64-linux-gnu/libc.so.6+0x42596) (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) SUMMARY: AddressSanitizer: heap-use-after-free /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33 in main Shadow bytes around the buggy address: 0x513ffffffd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x513ffffffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x513ffffffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x513fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x513fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x514000000000: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd 0x514000000080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x514000000100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x514000000180: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa 0x514000000200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x514000000280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 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 Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 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 ==879133==ABORTING Aborted With best regards, Dmitry.
On 11.11.2024 16:35, Дмитрий Фролов wrote: > > > On 11.11.2024 15:51, Prasad Pandit wrote: >> On Mon, 11 Nov 2024 at 17:41, Дмитрий Фролов <frolov@swemel.ru> wrote: >>> Above loop dereferences the pointer env, which is pointing to >>> the memory area, which is not allowed to read. >> * Not allowed to read environment variables? Is it because >> Debian/clang does not support the '**envp' parameter? Is '**envp' set >> to NULL on Debian? If '**envp' is not supported, then the compiler >> should throw an error at build time, no? > Not allowed to read the exact memory area, because it is marked as freed. As far as I understand, heap-use-after-free means a situation when code allocates memory then frees it and then access it. Here the code just accesses invalid memory because of nonstandard main() call convention. If it is correct, the patch title could be "tests/qtest: make access to environment variables portable" -- Alexey >>> I am pointing on 2 facts: >>> 1. "env" is Microsoft`s extension, not a standard >>> 2. There is exact example, where standards violation raises >>> undefined behavior: debian13/clang16 >>> >> * If this is about Debian not supporting '**envp' parameter, then >> it'll help if the commit message says "...Debian does not support this >> non-standard extension and crashes QEMU". > Since this is UB, it does not matter, if a crash happens or not. > ASAN just helps to highlight the hidden problem. > >> The asan error makes it >> sound like the patch fixes the use-after-free issue. > I didn`t want to confuse anybody, but this is exactly, > what is actually happening (see log below). > >> What happens if >> the -lasan is not used? Does it still crash QEMUt? >> >> Thank you. >> --- >> - Prasad >> > When saintizers are disabled, qos-test passes successfully. > qos-test fails when qemu is built with enabled sanitizers > (--enable-asan --enable-ubsan) > > ==879133==ERROR: AddressSanitizer: heap-use-after-free on address > 0x514000000040 at pc 0x55eae79b407c bp 0x7ffd028715d0 sp 0x7ffd028715c8 > READ of size 8 at 0x514000000040 thread T0 > #0 0x55eae79b407b in main > /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33 > #1 0x7f9011760c89 (/lib/x86_64-linux-gnu/libc.so.6+0x27c89) > (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) > #2 0x7f9011760d44 in __libc_start_main > (/lib/x86_64-linux-gnu/libc.so.6+0x27d44) (BuildId: > 61cf5c68463ab7677fa14f071a036eda24d0cc38) > #3 0x55eae77a5c60 in _start > (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x209c60) > (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) > > 0x514000000040 is located 0 bytes inside of 416-byte region > [0x514000000040,0x5140000001e0) > freed by thread T0 here: > #0 0x55eae7840ce9 in __interceptor_realloc > (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) > (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) > #1 0x7f901177b596 (/lib/x86_64-linux-gnu/libc.so.6+0x42596) > (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) > > previously allocated by thread T0 here: > #0 0x55eae7840ce9 in __interceptor_realloc > (/home/df/projects/upstream/qemu/build/tests/qtest/qos-test+0x2a4ce9) > (BuildId: 2c9032193c32f574ceec39c89e10b1693e20d69e) > #1 0x7f901177b596 (/lib/x86_64-linux-gnu/libc.so.6+0x42596) > (BuildId: 61cf5c68463ab7677fa14f071a036eda24d0cc38) > > SUMMARY: AddressSanitizer: heap-use-after-free > /home/df/projects/upstream/qemu/build/../tests/qtest/qos-test.c:339:33 > in main > Shadow bytes around the buggy address: > 0x513ffffffd80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x513ffffffe00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x513ffffffe80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x513fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 0x513fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > =>0x514000000000: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd > 0x514000000080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x514000000100: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd > 0x514000000180: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa > 0x514000000200: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 > 0x514000000280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > 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 > Freed heap region: fd > Stack left redzone: f1 > Stack mid redzone: f2 > Stack right redzone: f3 > 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 > ==879133==ABORTING > Aborted > > > With best regards, > Dmitry. > > _______________________________________________ > sdl.qemu mailing list > sdl.qemu@linuxtesting.org > http://linuxtesting.org/cgi-bin/mailman/listinfo/sdl.qemu
Hi, On Mon, 11 Nov 2024 at 22:51, Alexey Khoroshilov <khoroshilov@ispras.ru> wrote: > On 11.11.2024 16:35, Дмитрий Фролов wrote: > Not allowed to read the exact memory area, because it is marked as freed. > > As far as I understand, heap-use-after-free means a situation when code allocates memory then frees it and then access it. > Here the code just accesses invalid memory because of nonstandard main() call convention. > > If it is correct, the patch title could be "tests/qtest: make access to environment variables portable" ... > Since this is UB, it does not matter, if a crash happens or not. > ASAN just helps to highlight the hidden problem. * It is not clear what is happening here. The third parameter (**envp) looks widely supported. -> https://www.gnu.org/software/libc/manual/html_node/Program-Arguments.html Above document says and following program confirms '**envp' points to the same address as '*environ' variable. It also says '**envp' is not portable. === #define _GNU_SOURCE #include <stdio.h> #include <unistd.h> int main(int argc, char *argv[], char **envp) { printf("environ: %p, envp: %p\n", environ, envp); return 0; } $ cc -xc -o test test.c -lasan $ ./test environ: 0x7ffc5eb12168, envp: 0x7ffc5eb12168 === > When saintizers are disabled, qos-test passes successfully. > qos-test fails when qemu is built with enabled sanitizers * That means Debian/clang has no qualms about the third parameter. It is not a portability issue then. This Debian page also indicates usage of '**envp' parameter -> https://www.debian.org/doc/manuals/debian-reference/ch12.en.html. * If both '*environ' and '**envp' point to the same address, why does ASAN throw error with one and not with the other? Where is that memory getting free'd? * The patch looks fairly innocuous, but along with the commit message it is confusing enough to review it. I'd be okay to review it if we get some clarity about what is going on there. Thank you. --- - Prasad
diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c index 114f6bef27..e8ac00f0f7 100644 --- a/tests/qtest/qos-test.c +++ b/tests/qtest/qos-test.c @@ -326,7 +326,7 @@ static void walk_path(QOSGraphNode *orig_path, int len) * machine/drivers/test objects * - Cleans up everything */ -int main(int argc, char **argv, char** envp) +int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); @@ -336,7 +336,7 @@ int main(int argc, char **argv, char** envp) if (g_test_verbose()) { qos_printf("ENVIRONMENT VARIABLES: {\n"); - for (char **env = envp; *env != 0; env++) { + for (char **env = environ; *env != 0; env++) { qos_printf("\t%s\n", *env); } qos_printf("}\n");
"int main(int argc, char **argv, char** envp)" is non-standart Microsoft`s extention of the C language and it`s not portable. In my particular case (Debian 13, clang-16) this raises wild-pointer dereference with ASAN message "heap-use-after-free". Signed-off-by: Dmitry Frolov <frolov@swemel.ru> --- tests/qtest/qos-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)