On 2024/07/10 17:02, hellord wrote:
On Tue, Jul 9, 2024 at 10:39 PM Peter Maydell
<peter.mayd...@linaro.org <mailto:peter.mayd...@linaro.org>> wrote:
> +#define MAX_CHUNK_SIZE (128 * 1024 * 1024)
I think we could add a comment here, something like:
/*
* Maximum size to fwrite() to the output file at once;
* the MSVCRT runtime will not correctly handle fwrite()
* of more than 4GB at once.
*/
That will act as a reminder about why we do it.
Thanks, I agree.
(Does the library fwrite fail for > 4GB, or for >= 4GB ?
Your commit message says the former, so I've gone with that,
but if it's an "overflows 32 bit variable" kind of bug then
4GB exactly probably also doesn't work.)
It fails for > 4GB.
The msvcrt.dll!fwrite(buff, (4G+0x1000), 1, file) works as following:
(based on assembly, not the original source, irrelevant parts have been
omitted)
size_t fwrite(const void* buff, size_t element_size, size_t
element_count, FILE* file_p)
{
size_t size_t_total_size = element_size * element_count;
size_t size_t_remain_size = size_t_total_size;
unsigned int u32_written_bytes;
unsigned int buf_size;
/* The register used is r12d but not r12.
* So I suspect that Microsoft wrote it as an unsigned int type
* (or msvc compiler bug? seems unlikely)
*/
unsigned int u32_chunk_size;
while (true) {
if ((file_p->flags & 0x10C) != 0) {
buf_size = file_p->buf_size;
}
else {
// Always reaches here on the first fwrite() after fopen().
buf_size = 4096; // mov r15d, 1000h
}
if (size_t_remain_size > buf_size) {
u32_chunk_size = size_t_remain_size;
if (buf_size) {
// div ... ; sub r12d,edx; size_t stored into r12d ,
lost high 32 bits
// u32_chunk_size = 0x100000FFF - 0x100000FFF % 0x1000
// = 0x100000FFF - 0xFFF
// = 0x1 0000 0000
// = (u32) 0x1 0000 0000
// = 0
u32_chunk_size = size_t_remain_size -
(size_t_remain_size % buf_size);
}
//call _write() with zero size, returns 0
u32_written_bytes = __write(file_p, data_buff, u32_chunk_size);
// They didn't check if __write() returns 0.
if (u32_written_bytes == -1 || u32_written_bytes <
u32_chunk_size) {
return (size_t_total_size - size_t_remain_size) /
element_size;
}
//size_t_remain_size -= 0
size_t_remain_size -= u32_written_bytes;
buff += u32_written_bytes;
//size_t_remain_size will never decrease to zero, then
while(true) loop forever.
if (size_t_remain_size == 0) {
return element_count;
}
// ...
}
else {
// ...
}
}
// ...
}
note:
1. The path of buggy msvcrt.dll is c:\windows\system32\msvcrt.dll(
mingw64 links to it );
2. fwrite implementation in another msvc library which is called
ucrtbase.dll is correct(msvc links to it by default).
I don't object to this change but you should use ucrt whenever possible.
I'm not confident that elf2dmp and other QEMU binaries would work well
with mvcrt.
I even would like to force users to use ucrt and call setlocale(LC_ALL,
".UTF8") to fix text encoding, but I haven't done that yet because
Fedora, which cross-compiles QEMU for CI, still uses msvcrt.
Regards,
Akihiko Odaki
