Closed
Bug 1295103
Opened 8 years ago
Closed 8 years ago
Use MOZ_MUST_USE in OriginAttributes
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: kanru, Assigned: kanru)
References
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
allstars.chh
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mayhemer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mayhemer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mayhemer
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
smaug
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
allstars.chh
:
review+
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kchen
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8781085 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild. https://reviewboard.mozilla.org/r/71590/#review69182 with returning false, r+ ::: dom/ipc/ContentChild.cpp:2440 (Diff revision 1) > // note we do not need to force mUserContextId to the default here because > // the permission manager does that internally. > nsAutoCString originNoSuffix; > PrincipalOriginAttributes attrs; > - attrs.PopulateFromOrigin(permission.origin, originNoSuffix); > + bool success = attrs.PopulateFromOrigin(permission.origin, originNoSuffix); > + NS_ENSURE_TRUE(success, true); Shouldn't you return false here. I'd expect this kind of failure to be rather fatal.
Attachment #8781085 -
Flags: review?(bugs) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8781081 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext.. https://reviewboard.mozilla.org/r/71580/#review69302
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8781081 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext.. https://reviewboard.mozilla.org/r/71580/#review69304
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8781086 [details] Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes. https://reviewboard.mozilla.org/r/71592/#review69306
Attachment #8781086 -
Flags: review?(allstars.chh) → review+
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8781081 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext.. https://reviewboard.mozilla.org/r/71582/#review69308
Attachment #8781081 -
Flags: review?(allstars.chh) → review+
![]() |
||
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8781083 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in nsCookieService. https://reviewboard.mozilla.org/r/71586/#review70290 ::: netwerk/cookie/nsCookieService.cpp:2805 (Diff revision 1) > > nsAutoCString suffix; > NeckoOriginAttributes attrs; > stmt->GetUTF8String(IDX_ORIGIN_ATTRIBUTES, suffix); > - attrs.PopulateFromSuffix(suffix); > + if (!attrs.PopulateFromSuffix(suffix)) { > + return; why return and not continue? I think if this fails, the database might be corrupted? ehsan knows this code better.
Attachment #8781083 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8781082 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread https://reviewboard.mozilla.org/r/71584/#review70288
Attachment #8781082 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8781084 [details] Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier. https://reviewboard.mozilla.org/r/71588/#review70294
Attachment #8781084 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #12) > Comment on attachment 8781083 [details] > Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in > nsCookieService. > > https://reviewboard.mozilla.org/r/71586/#review70290 > > ::: netwerk/cookie/nsCookieService.cpp:2805 > (Diff revision 1) > > > > nsAutoCString suffix; > > NeckoOriginAttributes attrs; > > stmt->GetUTF8String(IDX_ORIGIN_ATTRIBUTES, suffix); > > - attrs.PopulateFromSuffix(suffix); > > + if (!attrs.PopulateFromSuffix(suffix)) { > > + return; > > why return and not continue? > > I think if this fails, the database might be corrupted? ehsan knows this > code better. Actually now I'm not sure what to do here. Looks like we should at least call HandleCorruptDB. Ehsan, what should I do to handle the case if OriginAttributes is not valid?
Flags: needinfo?(ehsan)
Comment 16•8 years ago
|
||
I commented on ReviewBoard but it is not reflected here: Hmm, what are the cases in whcih we'd fail to parse the OA? The only reasonable case I can think of is when you open a cookie DB from a newer version of Firefox which has added an origin attribute in an older version which doesn't support that attribute, which would cause PopulateFromSuffixIterator::URLParamsIterator() to return false. I don't think it's reasonable to trigger a DB rebuild in that case! The only thing that would sort of make sense in that scenario is to ignore the OA attributes that we don't support, which basically amounts to not checking the return value here.
Flags: needinfo?(ehsan)
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to :Ehsan Akhgari from comment #16) > I commented on ReviewBoard but it is not reflected here: > > Hmm, what are the cases in whcih we'd fail to parse the OA? The only > reasonable case I can think of is when you open a cookie DB from a newer > version of Firefox which has added an origin attribute in an older version > which doesn't support that attribute, which would cause > PopulateFromSuffixIterator::URLParamsIterator() to return false. I don't > think it's reasonable to trigger a DB rebuild in that case! > > The only thing that would sort of make sense in that scenario is to ignore > the OA attributes that we don't support, which basically amounts to not > checking the return value here. Thanks, Ehsan! I will go the not checking the return value route.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•8 years ago
|
||
Pushed by kchen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/83fe5575f4bb Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext.. r=allstars https://hg.mozilla.org/integration/autoland/rev/1e0a07384496 Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread r=mayhemer https://hg.mozilla.org/integration/autoland/rev/f1df7e0face1 Check OriginAttributes::PopulateFromSuffix return value in nsCookieService. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/4457d1e15ac8 Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/22a24db4bb36 Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild. r=smaug https://hg.mozilla.org/integration/autoland/rev/f11c01bf62a1 Use MOZ_MUST_USE in OriginAttributes. r=allstars
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/83fe5575f4bb https://hg.mozilla.org/mozilla-central/rev/1e0a07384496 https://hg.mozilla.org/mozilla-central/rev/f1df7e0face1 https://hg.mozilla.org/mozilla-central/rev/4457d1e15ac8 https://hg.mozilla.org/mozilla-central/rev/22a24db4bb36 https://hg.mozilla.org/mozilla-central/rev/f11c01bf62a1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•