1295103 - Use MOZ_MUST_USE in OriginAttributes

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

Review commit: https://reviewboard.mozilla.org/r/71582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71582/

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread

Review commit: https://reviewboard.mozilla.org/r/71584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71584/

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in nsCookieService.

Review commit: https://reviewboard.mozilla.org/r/71586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71586/

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier.

Review commit: https://reviewboard.mozilla.org/r/71588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71588/

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild.

Review commit: https://reviewboard.mozilla.org/r/71590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71590/

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes.

Review commit: https://reviewboard.mozilla.org/r/71592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71592/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69302

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69304

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Comment on attachment 8781086 [details]
Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes.

https://reviewboard.mozilla.org/r/71592/#review69306

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71582/#review69308

Comment 12

[Honza Bambas (:mayhemer)]

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.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8781082 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread

https://reviewboard.mozilla.org/r/71584/#review70288

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8781084 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier.

https://reviewboard.mozilla.org/r/71588/#review70294

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

(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?

Comment 16

[(no longer active)]

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.

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

(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 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71582/diff/1-2/

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781082 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71584/diff/1-2/

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781083 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in nsCookieService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71586/diff/1-2/

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781084 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71588/diff/1-2/

Comment 22

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781085 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71590/diff/1-2/

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781086 [details]
Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71592/diff/1-2/

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71582/diff/2-3/

Comment 25

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781082 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in DOMStorageDBThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71584/diff/2-3/

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781083 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in nsCookieService.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71586/diff/2-3/

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781084 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in PackagedAppVerifier.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71588/diff/2-3/

Comment 28

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781085 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in TabContext and ContentChild.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71590/diff/2-3/

Comment 29

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8781086 [details]
Bug 1295103 - Use MOZ_MUST_USE in OriginAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71592/diff/2-3/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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