1501808 - CacheFileMetadata::GetElement+0x51

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

Description

[Tom Ritter [:tjr]]

Attachment: Sample Crash 1.txt

We crash right here: https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/netwerk/cache2/CacheFileMetadata.cpp#396 because data/mBuf is NULL.

Using this build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a430539f8bed766f1bcd00a13b629fb89f72d1&selectedJob=207371345 (x64 opt) opening this page: https://browserbench.org/ARES-6

I don't know if this is related to Bug 1501799 - they both seem to involve file access stuff.  I'm not sure how well supported --disable-sandbox is on Windows; it might even be an error with that. I'm going to try a build with sandbox enabled and see what happens....

Comment 1

[Tom Ritter [:tjr]]

Same (for both bugs) with --enable-sandbox; so seems unrelated.

Comment 2

[Tom Ritter [:tjr]]

Backtracked this crash a bunch. I'm getting a string that contains 1;%64d,blahblahblah.  That shouldn't be %64d, that should be a number.

This line right here: https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/xpcom/string/nsTSubstring.h#727

I placed breakpoints on all of these and inspected the format strings. They're %I64d and %I64u...

Comment 3

[(not currently active) Ted Mielczarek]

Apparently Microsoft allows %I64d in printf format strings:
https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx

But nsTSubstring::AppendPrintf uses this code, which doesn't look like it supports that format specifier:
https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/mozglue/misc/Printf.cpp#549

Comment 4

[Tom Ritter [:tjr]]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #3)
> Apparently Microsoft allows %I64d in printf format strings:
> https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx
> 
> But nsTSubstring::AppendPrintf uses this code, which doesn't look like it
> supports that format specifier:
> https://searchfox.org/mozilla-central/rev/
> d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/mozglue/misc/Printf.cpp#549

clang-cl uses different format specifiers than mingw: https://ritter.vg/misc/transient/format-specifiers-diff.html

Comment 5

[Tom Ritter [:tjr]]

I wrote a MinGW patch that changes the format specifiers, and that fixes it!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db7504d12cf2e5181f2fcd46e09df37e0e80159

Even with the sandbox, I've run all four tests on browserbench without any crash.

Once we find out if MinGW wants to upstream my patch or something equivalent; we can take action here.

Comment 6

[Jacek Caban]

I just pushed the fix to upstream mingw-w64.

Comment 7

[Tom Ritter [:tjr]]

Attachment: Bug 1501808 - Bump MinGW Version to capture several MinGW fixes

Comment 8

[(Away)]

Comment on attachment 9022237 [details] [diff] [review]
Bug 1501808 - Bump MinGW Version to capture several MinGW fixes

70860d94 (HEAD -> master, origin/master, origin/HEAD) inttypes.h: Take into account __USE_MINGW_ANSI_STDIO and msvcrt version instead of depending on _mingw_print_p*.h headers.
b32ed6fb winternl.h: Added PUBLIC_OBJECT_TYPE_INFORMATION declaration.
ed8173d5 Updated imported headers to current Wine version.
e3542077 widl: Updated to current Wine version.
a91a7e40 _mingw_mac.h: Enable WIDL_EXPLICIT_AGGREGATE_RETURNS workaround for ABI incompatibility on affected platforms
8bba2a9f winpthreads: make winpthreads compatible with libcxx __attribute__((__require_constant_initialization__))
c69c7a70 headers: Update the threadlocinfo struct for ucrt

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c25ac5d5bb
Bump MinGW Version to capture several MinGW fixes. r=dmajor

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e8c25ac5d5bb