Repository navigation
Stack corruption with snprintf(3) and format %hu #5130
Description
Activity
Debugging on
NetBSD 7.99.26With the following patch I reduce the number of falling down tests (
alloca(3) fixes for NetBSD locally committed) from ~63 to 7.diff --git a/src/pal/src/cruntime/printfcpp.cpp b/src/pal/src/cruntime/printfcpp.cpp index 84c003b..9c41b7a 100644 --- a/src/pal/src/cruntime/printfcpp.cpp +++ b/src/pal/src/cruntime/printfcpp.cpp @@ -1397,85 +1397,113 @@ int CoreVfwprintf(CPalThread *pthrCurrent, PAL_FILE *stream, const wchar_16 *for if (Type == PFF_TYPE_P && Prefix == PFF_PREFIX_SHORT) { // Convert from pointer -> int -> short to avoid warnings. long trunc1; short trunc2; trunc1 = va_arg(ap, LONG); trunc2 = (short)trunc1; trunc1 = trunc2; TempInt = snprintf(TempSprintfStr, TEMP_COUNT, TempBuff, trunc1); if (TempInt < 0 || static_cast<size_t>(TempInt) >= TEMP_COUNT) { if (NULL == (TempSprintfStrPtr = (char*)InternalMalloc(++TempInt))) { ERROR("InternalMalloc failed\n"); LOGEXIT("vfwprintf returns int -1\n"); PERF_EXIT(vfwprintf); pthrCurrent->SetLastError(ERROR_NOT_ENOUGH_MEMORY); va_end(ap); return -1; } TempSprintfStr = TempSprintfStrPtr; snprintf(TempSprintfStr, TempInt, TempBuff, trunc2); } } else if (Type == PFF_TYPE_INT && Prefix == PFF_PREFIX_SHORT) { +#if 0 // Convert explicitly from int to short to get // correct sign extension for shorts on all systems. int n; short s; n = va_arg(ap, int); s = (short) n; TempInt = snprintf(TempSprintfStr, TEMP_COUNT, TempBuff, s); if (TempInt < 0 || static_cast<size_t>(TempInt) >= TEMP_COUNT) { if (NULL == (TempSprintfStrPtr = (char*)InternalMalloc(++TempInt))) { ERROR("InternalMalloc failed\n"); LOGEXIT("vfwprintf returns int -1\n"); PERF_EXIT(vfwprintf); pthrCurrent->SetLastError(ERROR_NOT_ENOUGH_MEMORY); va_end(ap); return -1; } TempSprintfStr = TempSprintfStrPtr; snprintf(TempSprintfStr, TempInt, TempBuff, s); } +#else + va_list apcopy; + + va_copy(apcopy, ap); + TempInt = vsnprintf(TempSprintfStr, TEMP_COUNT, TempBuff, apcopy); + va_end(apcopy); + PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); + + if (TempInt < 0 || static_cast<size_t>(TempInt) >= TEMP_COUNT) + { + if (NULL == (TempSprintfStrPtr = (char*)InternalMalloc(++TempInt))) + { + ERROR("InternalMalloc failed\n"); + LOGEXIT("vfwprintf returns int -1\n"); + PERF_EXIT(vfwprintf); + pthrCurrent->SetLastError(ERROR_NOT_ENOUGH_MEMORY); + va_end(ap); + return -1; + } + + TempSprintfStr = TempSprintfStrPtr; + va_copy(apcopy, ap); + vsnprintf(TempSprintfStr, TempInt, TempBuff, apcopy); + va_end(apcopy); + PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); + } +#endif } else { va_list apcopy; va_copy(apcopy, ap); TempInt = vsnprintf(TempSprintfStr, TEMP_COUNT, TempBuff, apcopy); va_end(apcopy); PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); if (TempInt < 0 || static_cast<size_t>(TempInt) >= TEMP_COUNT) { if (NULL == (TempSprintfStrPtr = (char*)InternalMalloc(++TempInt))) { ERROR("InternalMalloc failed\n"); LOGEXIT("vfwprintf returns int -1\n"); PERF_EXIT(vfwprintf); pthrCurrent->SetLastError(ERROR_NOT_ENOUGH_MEMORY); va_end(ap); return -1; } TempSprintfStr = TempSprintfStrPtr; va_copy(apcopy, ap); vsnprintf(TempSprintfStr, TempInt, TempBuff, apcopy); va_end(apcopy); PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); } } @@ -1784,69 +1812,77 @@ int CoreVsnprintf(CPalThread *pthrCurrent, LPSTR Buffer, size_t Count, LPCSTR Fo // Types that sprintf can handle size_t TempCount = Count - (BufferPtr - Buffer); #if !HAVE_LARGE_SNPRINTF_SUPPORT // Limit TempCount to 0x40000000, which is sufficient // for platforms on which snprintf fails for very large // sizes. if (TempCount > 0x40000000) { TempCount = 0x40000000; } #endif // HAVE_LARGE_SNPRINTF_SUPPORT TempInt = 0; // %h (short) doesn't seem to be handled properly by local sprintf, // so we do the truncation ourselves for some cases. if (Type == PFF_TYPE_P && Prefix == PFF_PREFIX_SHORT) { // Convert from pointer -> int -> short to avoid warnings. long trunc1; short trunc2; trunc1 = va_arg(ap, LONG); trunc2 = (short) trunc1; trunc1 = trunc2; TempInt = snprintf(BufferPtr, TempCount, TempBuff, trunc1); } else if (Type == PFF_TYPE_INT && Prefix == PFF_PREFIX_SHORT) { +#if 0 // Convert explicitly from int to short to get // correct sign extension for shorts on all systems. int n; short s; n = va_arg(ap, int); s = (short) n; TempInt = snprintf(BufferPtr, TempCount, TempBuff, s); +#else + va_list apcopy; + va_copy(apcopy, ap); + TempInt = vsnprintf(BufferPtr, TempCount, TempBuff, apcopy); + va_end(apcopy); + PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); +#endif } else { va_list apcopy; va_copy(apcopy, ap); TempInt = vsnprintf(BufferPtr, TempCount, TempBuff, apcopy); va_end(apcopy); PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); } if (TempInt < 0 || static_cast<size_t>(TempInt) >= TempCount) /* buffer not long enough */ { BufferPtr += TempCount; BufferRanOut = TRUE; } else { BufferPtr += TempInt; } } } else { *BufferPtr++ = *Fmt++; /* copy regular chars into buffer */ } } if (static_cast<int>(Count) > (BufferPtr - Buffer)) //Count is assumed to be in the range of int { *BufferPtr = 0; /* end the string */ @@ -2082,69 +2118,77 @@ int CoreWvsnprintf(CPalThread *pthrCurrent, LPWSTR Buffer, size_t Count, LPCWSTR and place them in the buffer (BufferPtr) */ size_t TempCount = Count - (BufferPtr - Buffer); TempInt = 0; #if !HAVE_LARGE_SNPRINTF_SUPPORT // Limit TempCount to 0x40000000, which is sufficient // for platforms on which snprintf fails for very large // sizes. if (TempCount > 0x40000000) { TempCount = 0x40000000; } #endif // HAVE_LARGE_SNPRINTF_SUPPORT // %h (short) doesn't seem to be handled properly by local sprintf, // so we do the truncation ourselves for some cases. if (Type == PFF_TYPE_P && Prefix == PFF_PREFIX_SHORT) { // Convert from pointer -> int -> short to avoid warnings. long trunc1; short trunc2; trunc1 = va_arg(ap, LONG); trunc2 = (short)trunc1; trunc1 = trunc2; TempInt = snprintf((LPSTR)BufferPtr, TempCount, TempBuff, trunc1); } else if (Type == PFF_TYPE_INT && Prefix == PFF_PREFIX_SHORT) { +#if 0 // Convert explicitly from int to short to get // correct sign extension for shorts on all systems. int n; short s; n = va_arg(ap, int); s = (short) n; TempInt = snprintf((LPSTR)BufferPtr, TempCount, TempBuff, s); +#else + va_list apcopy; + va_copy(apcopy, ap); + TempInt = vsnprintf((LPSTR) BufferPtr, TempCount, TempBuff, apcopy); + va_end(apcopy); + PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); +#endif } else { va_list apcopy; va_copy(apcopy, ap); TempInt = vsnprintf((LPSTR) BufferPtr, TempCount, TempBuff, apcopy); va_end(apcopy); PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); } if (TempInt == 0) { // The argument is "". continue; } if (TempInt < 0 || static_cast<size_t>(TempInt) >= TempCount) /* buffer not long enough */ { TempNumberBuffer = (LPSTR) InternalMalloc(TempCount+1); if (!TempNumberBuffer) { ERROR("InternalMalloc failed\n"); pthrCurrent->SetLastError(ERROR_NOT_ENOUGH_MEMORY); errno = ENOMEM; va_end(ap); return -1; } if (strncpy_s(TempNumberBuffer, TempCount+1, (LPSTR) BufferPtr, TempCount) != SAFECRT_SUCCESS) { ASSERT("strncpy_s failed!\n"); @@ -2463,69 +2507,77 @@ int CoreVfprintf(CPalThread *pthrCurrent, PAL_FILE *stream, const char *format, if (-1 == paddingReturnValue) { ERROR("Internal_AddPaddingVfprintf failed\n"); PERF_EXIT(vfprintf); va_end(ap); return -1; } written += paddingReturnValue; } else { // Types that fprintf can handle. TempInt = 0; // %h (short) doesn't seem to be handled properly by local sprintf, // so we do the truncation ourselves for some cases. if (Type == PFF_TYPE_P && Prefix == PFF_PREFIX_SHORT) { // Convert from pointer -> int -> short to avoid warnings. long trunc1; short trunc2; trunc1 = va_arg(ap, LONG); trunc2 = (short)trunc1; trunc1 = trunc2; TempInt = fprintf(stream->bsdFilePtr, TempBuff, trunc1); } else if (Type == PFF_TYPE_INT && Prefix == PFF_PREFIX_SHORT) { +#if 0 // Convert explicitly from int to short to get // correct sign extension for shorts on all systems. int n; short s; n = va_arg(ap, int); s = (short) n; TempInt = fprintf( stream->bsdFilePtr, TempBuff, s); +#else + va_list apcopy; + va_copy(apcopy, ap); + TempInt = vfprintf(stream->bsdFilePtr, TempBuff, apcopy); + va_end(apcopy); + PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); +#endif } else { va_list apcopy; va_copy(apcopy, ap); TempInt = vfprintf(stream->bsdFilePtr, TempBuff, apcopy); va_end(apcopy); PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); } if (-1 == TempInt) { ERROR("vfprintf returned an error\n"); } else { written += TempInt; } } } else { #if FILE_OPS_CHECK_FERROR_OF_PREVIOUS_CALL clearerr (stream->bsdFilePtr); #endif InternalFwrite(Fmt++, 1, 1, stream->bsdFilePtr, &stream->PALferrorCode); /* copy regular chars into buffer */ if (stream->PALferrorCode == PAL_FILE_ERROR) {PAL test results with the above patch applied.
Finished running PAL tests. The following test(s) failed: exception_handling/pal_sxs/test1/paltest_pal_sxs_test1. Exit code: 134 threading/CriticalSectionFunctions/test2/paltest_criticalsectionfunctions_test2. Exit code: 134 threading/CriticalSectionFunctions/test4/paltest_criticalsectionfunctions_test4. Exit code: 134 threading/DuplicateHandle/test8/paltest_duplicatehandle_test8. Exit code: 134 threading/GetCurrentThread/test1/paltest_getcurrentthread_test1. Exit code: 134 threading/GetCurrentThread/test2/paltest_getcurrentthread_test2. Exit code: 134 threading/ThreadPriority/test1/paltest_threadpriority_test1. Exit code: 134 PAL Test Results: Passed: 797 Failed: 7Looking at the function that converts the formatting string, the "%h" seems to be replaced incorrectly just by "%", but even %h alone is an incorrect specifier. So it is not used in the formatting and that's likely the reason why it doesn't crash while the "%hd" / "%hu" do.
Could you please paste here the whole formatting string passed as the "format" parameter to the CoreVfwprintf function when the crash happens?
It crashes with the following example formatting strings:
"foo %hu""foo %hd"
While it works with
"foo %h","foo %d", etc. Please see the number of passed tests and their coverage.@krytarowski I have stepped through the code on Linux and I have not noticed anything suspicious, except for one thing that depending on the va_arg definition in the new clang might cause a problem. The code at line 1819 is asking for int parameter, while the actual one is short. Although I believe that shorts should be passed as 32 bit values on AMD64 too (and in this case, it should be passed in a register, based on the AMD64 ABI document), who knows what the va_arg does in such case.
Could you please disassemble the CoreVsnprintf function in the gdb using disass /m and post it somewhere so that I can compare it with the code from the clang 3.5 on Linux? The va_arg stuff is inlined, so it should be sufficient to spot potential culprit.Thank you @janvorli for help! Generally shorter types are promoted to longer.
stdarg(3) on NetBSD says:If the type in question is one that gets promoted, the promoted type should be used as the argument to va_arg(). The following describes which types are promoted (and to what): - short is promoted to int - float is promoted to double - char is promoted to intI'm suspecting a compiler bug.. I will try to generate assembly for comparison. I wasn't able to reproduce this issue with a small code example. Nothing interesting resides in
.ifiles.I've added
nopmarks, just in case of optimization to close the region between twovolatilefragments.--- src/pal/src/cruntime/printfcpp.cpp.orig 2016-01-28 19:04:13.000000000 +0000 +++ src/pal/src/cruntime/printfcpp.cpp @@ -1811,6 +1811,7 @@ int CoreVsnprintf(CPalThread *pthrCurren } else if (Type == PFF_TYPE_INT && Prefix == PFF_PREFIX_SHORT) { + __asm__ volatile("nop;nop;nop;nop;nop;"); // Convert explicitly from int to short to get // correct sign extension for shorts on all systems. int n; @@ -1820,6 +1821,7 @@ int CoreVsnprintf(CPalThread *pthrCurren s = (short) n; TempInt = snprintf(BufferPtr, TempCount, TempBuff, s); + __asm__ volatile("nop;nop;nop;nop;nop;"); } else {The result:
1813 { 1814 __asm__ volatile("nop;nop;nop;nop;nop;"); 0x0000000000412154 <+5508>: nop 0x0000000000412155 <+5509>: nop 0x0000000000412156 <+5510>: nop 0x0000000000412157 <+5511>: nop 0x0000000000412158 <+5512>: nop 0x0000000000412159 <+5513>: lea -0x430(%rbp),%rax 1815 // Convert explicitly from int to short to get 1816 // correct sign extension for shorts on all systems. 1817 int n; 1818 short s; 1819 1820 n = va_arg(ap, int); 0x0000000000412160 <+5520>: mov -0x430(%rbp),%ecx 0x0000000000412166 <+5526>: cmp $0x28,%ecx 0x0000000000412169 <+5529>: mov %rax,-0x788(%rbp) 0x0000000000412170 <+5536>: mov %ecx,-0x78c(%rbp) 0x0000000000412176 <+5542>: ja 0x4121a1 <CoreVsnprintf(CorUnix::CPalThread*, char*, unsigned long, char const*, __va_list_tag*)+5585> 0x000000000041217c <+5548>: mov -0x78c(%rbp),%eax 0x0000000000412182 <+5554>: movslq %eax,%rcx 0x0000000000412185 <+5557>: mov -0x788(%rbp),%rdx 0x000000000041218c <+5564>: add 0x10(%rdx),%rcx 0x0000000000412190 <+5568>: add $0x8,%eax 0x0000000000412193 <+5571>: mov %eax,(%rdx) 0x0000000000412195 <+5573>: mov %rcx,-0x798(%rbp) 0x000000000041219c <+5580>: jmpq 0x4121be <CoreVsnprintf(CorUnix::CPalThread*, char*, unsigned long, char const*, __va_list_tag*)+5614> 0x00000000004121a1 <+5585>: mov -0x788(%rbp),%rax 0x00000000004121a8 <+5592>: mov 0x8(%rax),%rcx 0x00000000004121ac <+5596>: mov %rcx,%rdx 0x00000000004121af <+5599>: add $0x8,%rcx 0x00000000004121b3 <+5603>: mov %rcx,0x8(%rax) 0x00000000004121b7 <+5607>: mov %rdx,-0x798(%rbp) 0x00000000004121be <+5614>: mov -0x798(%rbp),%rax 0x00000000004121c5 <+5621>: lea -0x410(%rbp),%rdx 0x00000000004121cc <+5628>: mov (%rax),%ecx 0x00000000004121ce <+5630>: mov %ecx,-0x530(%rbp) 1821 s = (short) n; 0x00000000004121d4 <+5636>: mov -0x530(%rbp),%ecx 0x00000000004121da <+5642>: mov %cx,%si 0x00000000004121dd <+5645>: mov %si,-0x532(%rbp) 1822 1823 TempInt = snprintf(BufferPtr, TempCount, TempBuff, s); 0x00000000004121e4 <+5652>: mov -0x460(%rbp),%rdi 0x00000000004121eb <+5659>: mov -0x520(%rbp),%rsi 0x00000000004121f2 <+5666>: movswl -0x532(%rbp),%ecx 0x00000000004121f9 <+5673>: mov $0x0,%al 0x00000000004121fb <+5675>: callq 0x404cf0 <snprintf@plt> 0x0000000000412200 <+5680>: mov %eax,-0x4f0(%rbp) 1824 __asm__ volatile("nop;nop;nop;nop;nop;"); 0x0000000000412206 <+5686>: nop 0x0000000000412207 <+5687>: nop 0x0000000000412208 <+5688>: nop 0x0000000000412209 <+5689>: nop 0x000000000041220a <+5690>: nop 1825 } 0x000000000041220b <+5691>: jmpq 0x412287 <CoreVsnprintf(CorUnix::CPalThread*, char*, unsigned long, char const*, __va_list_tag*)+5815> 0x0000000000412210 <+5696>: lea -0x450(%rbp),%rax 0x0000000000412217 <+5703>: lea -0x410(%rbp),%rdx 0x000000000041221e <+5710>: lea -0x430(%rbp),%rcx 1826 else 1827 { 1828 va_list apcopy; 1829 va_copy(apcopy, ap); 0x0000000000412225 <+5717>: mov %rax,%rsi 0x0000000000412228 <+5720>: mov 0x10(%rcx),%rdi 0x000000000041222c <+5724>: mov %rdi,0x10(%rsi) 0x0000000000412230 <+5728>: movups (%rcx),%xmm0 0x0000000000412233 <+5731>: movups %xmm0,(%rsi) 1830 TempInt = vsnprintf(BufferPtr, TempCount, TempBuff, apcopy); 0x0000000000412236 <+5734>: mov -0x460(%rbp),%rdi 0x000000000041223d <+5741>: mov -0x520(%rbp),%rsi 0x0000000000412244 <+5748>: mov %rax,%rcx 0x0000000000412247 <+5751>: callq 0x404c20 <vsnprintf@plt> 0x000000000041224c <+5756>: lea -0x430(%rbp),%rdi 0x0000000000412253 <+5763>: lea -0x450(%rbp),%rcx 0x000000000041225a <+5770>: mov %eax,-0x4f0(%rbp) 1831 va_end(apcopy); 1832 PAL_printf_arg_remover(&ap, Width, Precision, Type, Prefix); 0x0000000000412260 <+5776>: mov -0x474(%rbp),%esi 0x0000000000412266 <+5782>: mov -0x478(%rbp),%edx 0x000000000041226c <+5788>: mov -0x480(%rbp),%eax 0x0000000000412272 <+5794>: mov -0x47c(%rbp),%r8d 0x0000000000412279 <+5801>: mov %rcx,-0x7a0(%rbp) 0x0000000000412280 <+5808>: mov %eax,%ecx 0x0000000000412282 <+5810>: callq 0x407260 <PAL_printf_arg_remover(va_list*, INT, INT, INT, INT)> 0x0000000000412287 <+5815>: jmpq 0x41228c <CoreVsnprintf(CorUnix::CPalThread*, char*, unsigned long, char const*, __va_list_tag*)+5820> 1833@janvorli if you need assembly for full function, please let me know.
@janvorli if you need assembly for full function, please let me know.
@krytarowski I would really need the whole disass of the function, ideally without the added nops. I have tried to compare your piece of code with my one, it seems equivalent except for slightly different instruction ordering and registers allocations at few places and some RBP offsets. Having the whole function disass would enable me to figure out whether the offsets are wrong or they are caused just by some data structure being larger on NetBSD.
OK, Just a sec.
I've disassembled
CoreVsnprintfthis way:gdb -batch -ex 'file ./bin/obj/NetBSD.x64.Debug/src/pal/tests/palsuite/c_runtime/vsprintf/test11/paltest_vsprintf_test11' -ex 'disassemble CoreVsnprintf'The result is accessible here:
ftp://ftp.netbsd.org/pub/NetBSD/misc/kamil/coreclr-disasm-1.txt
26 remaining items
I have thought we already do the trick for the _vsnprintf, but now I can see we don't. So that should work.
Thanks! I will mimic
PAL_printf.I will give it a try.. just need to fix dotnet/coreclr#3219 as it broke on NetBSD...
Thank you.
- added 5 commits that reference this issue
on Feb 18, 2016 - added 2 commits that reference this issue
on Apr 13, 2016 - ghost locked as resolved and limited conversation to collaborators
on Jan 2, 2021
PAL tests fall down with
src/pal/tests/palsuite/c_runtime/vsprintf/test11/test11.cwith the following example call:
There is stack corruption here:
https://github.com/dotnet/coreclr/blob/master/src/pal/src/cruntime/printfcpp.cpp#L1822
I'm trying to determine what's going on.