795395 - Valgrind on tbpl detects: Invalid read of size 4 with nsGSettingsService on the stack

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: m-c changeset 895f66c4eada Valgrind stack with line numbers

Valgrind detects: Invalid read of size 4 with nsGSettingsService on the stack, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=15623197&tree=Firefox&full=1

Guessing Core: Widget: Gtk, please change component if necessary.

s-s because this is an invalid read. Suspecting it is a regression from bug 713802 which itself likely comes from bug 611953.

Comment 1

[Chris AtLee [:catlee]]

related to https://bugzilla.mozilla.org/show_bug.cgi?id=794174 perhaps?

Comment 2

[Chris Coulson]

Attachment: Fix invalid read in nsGSettingsService

I guess what happens is nsGSettingsService::Init() fails because the glib version is too old for this feature, which causes us to attempt to unload the library twice (once in Init() and again in the destructor). The attached 1-liner should fix that (untested here though, because this doesn't fail on my machine)

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Chris Coulson from comment #2)
> Created attachment 666001 [details] [diff] [review]
> Fix invalid read in nsGSettingsService

Perhaps you'd like to request for review on this patch?

Although GIO landed some time ago, it was only turned on by default yesterday, so setting flags accordingly.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Since it is an arbitrary read, assuming sec-critical worse-case, unless otherwise shown.

Comment 5

[Daniel Veditz [:dveditz]]

This functionality is not really under attacker control so sec-moderate is probably more appropriate.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e05d8c7fc54b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.