1358159 - Crash in GetLocaleInfoHelper() called from :intl::OSPreferences::ReadDateTimePattern

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-d939c8a3-c208-4ad9-92d3-944ee0170420.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170419030223: http://bit.ly/2pj9fjX

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb38d935d699e0529f9e0bb35578d381026415c4&tochange=c0ea5ed7f91a6be996a4a3c5ab25e2cdf6b4377e

ni on gandalf as maybe he can shed some light on the crash. Affects Tbird as well as FF.  Some of the comments mention crashing when downloading files.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ouch, that's beyond Gecko. Not sure what's going on here. I'll look into known crashes in Windows API but also NI Jonathan who may shed some light on how are we triggering the crash.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

There's not much on the web about crashes in this function :(

Also, the range is strange. There's nothing related to OSPreferences in that range.

My guess would be either bug 1355308 which landed on the 10th and switched Windows to use UserLocale, or bug 1354445 which landed on the 8th and switched callsites to use mozIntl.DateTimeFormat.

Not sure why crashes started only on 19th.

Comment 3

[Jonathan Kew [:jfkthame]]

Crashing with a write violation inside the call at https://hg.mozilla.org/mozilla-central/annotate/c0ea5ed7f91a/intl/locale/windows/OSPreferences_win.cpp#l172 suggests to me that the target string length was not long enough for the result that Windows is trying to write to it.

I wonder about the SetLength(len - 1) on the previous line; without some digging, I'm unsure whether this guarantees to allocate enough space for the null terminator as well as the actual character data. Maybe we need to SetLength(len) there, to ensure the terminator will fit when Windows writes the data to its output buffer, and then SetLength(len - 1) afterwards so that it isn't considered part of the actual string.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

What's the course of action then in such a case? I can't reproduce it locally, so I'd be operating blind if I were to write a patch that changes the SetLength.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can you advise or NI the right people? I'm concerned that while :jfkthame indicated a potential fix, I cannot reason about whether it will be a complete fix and I don't know how to reproduce it.

It puts me in a really bad position to pursue patching it.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> What's the course of action then in such a case? I can't reproduce it
> locally, so I'd be operating blind if I were to write a patch that changes
> the SetLength.

I took a (brief) look at the string code, and I think its behavior matches what the code here expects: SetLength(n) should ensure the string has enough capacity for n characters _plus_ a null terminator. If that's the case, my suggestion above may well be irrelevant anyway. :(

Comment 7

[Marcia Knous [:marcia]]

I think the best course is for someone to track down a better regression range. Adding the relevant keywords to trigger that action.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

I don't believe that the regression window is going to surprise us. We know when we landed this change - bug 1308329 - we just don't know how this code causes the crash. :(

Comment 9

[Marcia Knous [:marcia]]

It looks as if Comment 0 is incorrect in the date I see crashes start - I see crashes in 20170416030209 build, which is a few days earlier than what I report in Comment 0.

All crashes so far are Windows 10. User comments mention:

* majorgeeks.com.
* crashes on any download update attempts of sites 
* unable to download updates for privaZer and smart defrag
* downloading files

ni on Andrei to see if someone in QA can reproduce on Windows 10.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Lots of writes (and some reads) to bad addresses...  at least sec-high; could be sec-crit

First build with a hit is 20170419

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm still stuck with this. Not sure what should be the next step. We know it's a sec-high, we know when we introduced it, but we don't know how to fix it.

Comment 12

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I could not reproduce this issue, I tried downloading files and updates. I also noticed that the crash number remained the same (12). Am I missing something?

Comment 13

[Marcia Knous [:marcia]]

(In reply to Cristian Comorasu [:CristiComo] from comment #12)
> I could not reproduce this issue, I tried downloading files and updates. I
> also noticed that the crash number remained the same (12). Am I missing
> something?

I see 14 crashes/41 installations as of today.

Comment 14

[Nathan Froyd [:froydnj]]

I think Jonathan in comment 3 is right; the edge case here is that if len == 1, then we SetLength to zero, which sets the string to point at a shared, read-only buffer:

https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.cpp#684

So if we hand out a pointer to that, and GetLocaleInfoEx tries to write a null terminator to that string, we're going to try and write to read-only memory, which would be consistent with the crash reports here.

Zibi, can you implement the fix described in comment 3, please?

Comment 15

[Nathan Froyd [:froydnj]]

Debugging a minidump from any of these crashes should be enough to figure out what lengths we're setting the string to; I'd bet we're setting it to zero in all cases.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug1358159.diff

Sure I can. Here's the patch.

My problem is that I'm operating blindly. I have no idea if this is the bug and if this is fixing it and I don't know how to test it :(

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8866474 [details] [diff] [review]
bug1358159.diff

Review of attachment 8866474 [details] [diff] [review]:
-----------------------------------------------------------------

Can you modify your date/time format patterns in the Windows control panel so that they are just a single character each? It seems like that ought to reproduce this bug. (It seems a bit surprising if that would happen with any "normal" format settings, though.)

We should do the same thing at the other GetLocaleInfoEx() call-site in the `if (isTime) { ... }` block later in this method, too, as the same potential issue exists there AFAICS.

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Tried that, changed my short and long date to "d" and my time to "t".

Then launched Firefox nightly with browser console and typed:

"(Services.intl.createDateTimeFormat('en-US', {dateStyle: 'short'})).format(0)"

tried with different date styles and time styles. No crash.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug1358159.diff v2

Updated patch.

As explained in comment 18, I failed to reproduce the crash with the shortest possible date/time format (1 char), so I'm not sure if this patch fixes anything tbh.

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 8866518 [details] [diff] [review]
bug1358159.diff v2

Review of attachment 8866518 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, in theory the problem would arise if you could set the format to an empty string (so the length returned by Windows, including the terminator, would be 1), but the control panel doesn't let me do that. So I haven't been able to reproduce a failure here.

Still, we should take this patch in case it does ever happen that windows returns an empty string.

I suggest marking the bug leave-open when this lands, until we see whether it does have any effect on crash-stats.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug1358159.diff v3

Now with 100% more commit meta-data!

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bug1358159.diff v4

Now, with r= instead of r?

Sorry for the noise! I'm learning `hg export` instead of MR :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/380aff8f2a0c0875154adfd4ff459e00d7d6e131

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/380aff8f2a0c

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Seems like potentially this patch did fix this bug.

So far, since 13th no new reports except of one from 38esr which apparently had the same problem in the old code - https://crash-stats.mozilla.com/report/index/58f6a0a9-f347-49a4-919f-2d4ed0170515 :)

I'm going to keep watching this for another week and by Friday, if no new reports show up from recent builds I'll close it as fixed.

Comment 26

[Jonathan Kew [:jfkthame]]

AFAICS, the code in 38esr didn't have exactly -this- issue (attempting to write a null terminator to a non-writable zero-length string), as it passed a fixed local buffer when calling the Win32 APIs (see dateBuffer and timeBuffer at https://hg.mozilla.org/releases/mozilla-esr38/annotate/6afcba951b0f/intl/locale/windows/nsDateTimeFormatWin.cpp#l96).

So I think the 38esr crash suggests a different failure mode than what we (theoretically, at least) fixed here, though I can't really tell what went wrong there.

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm going to make it as fixed.

There are still new reports coming but all of them now are from 38 and 52/53. None from nightly and nothing related to the code we fixed here.