1392449 - Coverity report: null pointer dereference in BookmarksSessionHelper

Status: RESOLVED FIXED

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

Description

[:Grisha Kruglov]

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);

Comment 1

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 2

[Nick Alexander :nalexander [he/him]]

> 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!

Comment 3

[Nick Alexander :nalexander [he/him]]

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

Comment 4

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 5

[:Grisha Kruglov]

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

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(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 :)

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1392449 - introduce shouldReconcileRecords so reconcileRecords never returns null.

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

Comment 8

[:Grisha Kruglov]

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.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8902146 [details]
Bug 1392449 - introduce shouldReconcileRecords so reconcileRecords never returns null.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/286dcdb7136d
introduce shouldReconcileRecords so reconcileRecords never returns null. r=Grisha

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/286dcdb7136d