Closed Bug 1392449 Opened 2 years ago Closed 2 years ago

Coverity report: null pointer dereference in BookmarksSessionHelper

Categories

(Firefox for Android :: Android Sync, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Grisha, Assigned: markh, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(1 file)

RepositorySession's reconcileRecords might return null, but it's not checked in BookmarksSessionHelper's use of it.

Annotate RepositorySession's reconcileRecords as @Nullable and perform a null check in BookmarksSessionHelper. In case session's reconcileRecords returns null, return null as well - we check for it in SessionHelper's store runnable implementation.

Or, for bonus points, re-wire this so that we're not passing nulls around as a signaling mechanism, and doing all these ugly checks. 

 *** CID 163936:  Null pointer dereferences  (NULL_RETURNS)
/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java: 497 in org.mozilla.gecko.sync.repositories.android.BookmarksSessionHelper.reconcileRecords(org.mozilla.gecko.sync.repositories.domain.Record, org.mozilla.gecko.sync.repositories.domain.Record, long, long)()
491
492             final BookmarkRecord remote = (BookmarkRecord) remoteRecord;
493             final BookmarkRecord local = (BookmarkRecord) localRecord;
494
495             // For now we *always* use the remote record's children array as a starting point.
496             // We won't write it into the database yet; we'll record it and process as we go.
>>>     CID 163936:  Null pointer dereferences  (NULL_RETURNS)
>>>     Accessing field of null object "reconciled".
497             reconciled.children = remote.children;
498
499             // *Always* track folders, though: if we decide we need to reposition items, we'll
500             // untrack later.
501             if (reconciled.isFolder()) {
502                 session.trackRecord(reconciled);
Mentor: gkruglov
Whiteboard: [good first bug]
BookmarksSessionHelper.java also doesn't have this annotation, but after this change it will - so I'm guessing it should also grow this annotation?

SessionHelper.java declares reconcileRecords as abstract, but it doesn't have nullable - should it also grow that annotation? (This one seems less clear, as I suppose that *some* implementations may be nullable, but others should not)

Also, how can I run coverity locally to check I've fixed the issue and haven't introduced new ones?

(In reply to :Grisha Kruglov from comment #0)
> Or, for bonus points, re-wire this so that we're not passing nulls around as
> a signaling mechanism, and doing all these ugly checks. 

Can you clarify what you mean here? Maybe a boolean can[/should]ReconcileRecords or similar?
Flags: needinfo?(gkruglov)
> Also, how can I run coverity locally to check I've fixed the issue and
> haven't introduced new ones?

I don't think this is possible.  Coverity is a third-party service that Sylvestre Ledru has configured to run against Fennec.  I get emails about issues and redirect them manually -- there aren't very many, and they only email about new issues -- and I can't even find a link in my history now!
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #2)
> > Also, how can I run coverity locally to check I've fixed the issue and
> > haven't introduced new ones?
> 
> I don't think this is possible.  Coverity is a third-party service that
> Sylvestre Ledru has configured to run against Fennec.  I get emails about
> issues and redirect them manually -- there aren't very many, and they only
> email about new issues -- and I can't even find a link in my history now!

Sylvestre, do you know otherwise?
Flags: needinfo?(sledru)
We are running the free stuff of Coverity. We don't have the license to have local usage (we tried to get one but it was way too expensive)
Flags: needinfo?(sledru)
(In reply to Mark Hammond [:markh] from comment #1)
> Can you clarify what you mean here? Maybe a boolean
> can[/should]ReconcileRecords or similar?

I'm thinking that splitting this into something like "shouldReconcileRecord" and "reconcileRecord" is the cleanest minimal change here, and should tidy this up a bit.

I also have a feeling this will change heavily in the somewhat near future... But small improvements like these are still useful.
Flags: needinfo?(gkruglov)
(In reply to :Grisha Kruglov from comment #5)
> I'm thinking that splitting this into something like "shouldReconcileRecord"
> and "reconcileRecord" is the cleanest minimal change here, and should tidy
> this up a bit.

Thanks. Patch is coming up - even though it will be requesting review, please consider it more a feedback request as I have no idea what I'm doing :)
Assignee: nobody → markh
Comment on attachment 8902146 [details]
Bug 1392449 - introduce shouldReconcileRecords so reconcileRecords never returns null.

https://reviewboard.mozilla.org/r/173604/#review179300

LGTM, a straightforward transformation.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:129
(Diff revision 1)
>      abstract Record processBeforeInsertion(Record toProcess);
>  
>      abstract void insert(RepositorySessionStoreDelegate delegate, Record record)
>              throws NoGuidForIdException, NullCursorException,ParentNotFoundException;
>  
> +    private boolean shouldReconcileRecords(final Record remoteRecord,

super nit: can you move this method lower, to be with other 'privates'?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:306
(Diff revision 1)
>  
>                          // Populate more expensive fields prior to reconciling.
>                          existingRecord = transformRecord(existingRecord);
>  
> -                        // Implementation is expected to decide if it's necessary to "track" the record.
> -                        Record toStore = reconcileRecords(record, existingRecord, lastRemoteRetrieval, lastLocalRetrieval);
> +                        if (!shouldReconcileRecords(record, existingRecord)) {
> +                            Logger.debug(LOG_TAG, "shouldReconcileRecords returned false. Not inserting a record.");

nit: "...not processing a record.." is more accurate.
Attachment #8902146 - Flags: review?(gkruglov) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/286dcdb7136d
introduce shouldReconcileRecords so reconcileRecords never returns null. r=Grisha
https://hg.mozilla.org/mozilla-central/rev/286dcdb7136d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.