Closed Bug 1295103 Opened 4 years ago Closed 4 years ago

Use MOZ_MUST_USE in OriginAttributes

Categories

(Core :: Security: CAPS, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

No description provided.
Assignee: nobody → kchen
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 on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69302
Comment on attachment 8781081 [details]
Bug 1295103 - Check OriginAttributes::PopulateFromSuffix return value in SerializedLoadContext..

https://reviewboard.mozilla.org/r/71580/#review69304
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 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 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 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 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+
(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)
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)
(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.
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
You need to log in before you can comment on or make changes to this bug.