Here is code from gfxPlatform::GetPrefLangName: if (uint32_t(aLang) < uint32_t(eFontPrefLang_AllCount)) return gPrefLangNames[uint32_t(aLang)]; eFontPrefLang_AllCount is 32, whereas gPrefLangNames has 31 entries in it. Thus if you pass in 31, you'll end up reading past the end of gPrefLangNames. Maybe the check should be against eFontPrefLang_LangCount (which is 30) instead of eFontPrefLang_AllCount? Presumably a static assert could catch problems here in the future. At first I thought this was a regression from bug 213517, but it looks like that _removed_ an entry from eFontPrefLang, so I think it made it slightly better. I did a bit of digging, but I'm not sure when this was introduced. Coverity first noticed this in December 2012.

Looks to me like the simple solution here is to scrap the enum constants for the count of lang names, and just use ArraySize when referring to the array of strings.

Comment on attachment 8433999 [details] [diff] [review] use mozilla::ArrayLength for array size. Review of attachment 8433999 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense

https://hg.mozilla.org/mozilla-central/rev/cbb2e610473e Should this go to Aurora or anything?

(In reply to Wes Kocher (:KWierso) from comment #4) > https://hg.mozilla.org/mozilla-central/rev/cbb2e610473e > > Should this go to Aurora or anything? There's no compelling need for it to do so IMO. We may sometimes read an out-of-bounds value, but we don't do anything dangerous with it, AFAICS.

The next ESR is FF31 so does that make a more compelling case for uplifting this?

Any security risk here looks low (AFAICS), but the patch itself is also extremely low risk, so there's no reason -not- to take it.

Comment on attachment 8433999 [details] [diff] [review] use mozilla::ArrayLength for array size. Nominating for beta (this is currently on aurora) so that it'll go into ESR31. It's minor, but very safe, and nicer for future maintenance if the ESR code is correct and matches trunk code going forward. Approval Request Comment [Feature/regressing bug #]: Longstanding flaw, found by coverity analysis rather than observed buggy/crashy behavior. [User impact if declined]: Probably none; it's not entirely clear to me whether the error here can actually be reached from web content, nor that anything worse than perhaps choosing the wrong font would happen. But -perhaps- in exactly the right conditions, it'd be possible to reach an out-of-bounds read, and crash with a protection violation if the location happens to be unreadable. [Describe test coverage new/current, TBPL]: There's no directly testable behavior exposed here. (Only if it were thoroughly broken, font-related reftests would start to fail.) [Risks and why]: Minimal risk, just avoids potentially reading one element too far from a static array and getting junk. [String/UUID change made/needed]: none

Comment on attachment 8433999 [details] [diff] [review] use mozilla::ArrayLength for array size. As Lukas said, 31 is an ESR release. We are going to maintain it for a while.

Marking this as [qa-] due to the lack of STR/POC that could be used for verification. If anything can be provided to help test and verify, I'll happily go through it!