SQLiteBridge doesn't handle NULL's in query parameters

REOPENED
Assigned to

Status

()

Firefox for Android
General
REOPENED
6 years ago
6 years ago

People

(Reporter: wesj, Assigned: gcp)

Tracking

Trunk
Firefox 19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 affected, blocking-fennec1.0 -)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 614972 [details] [diff] [review]
Patch

I've seen SQLiteBridge crash a few times when Sync passes it NULL values in the parameters array for a query call. This may be because I have some advanced JNI logging turned on, but good to fix.

We can't really fix the WHERE argument they send to read WHERE something IS NULL, but we can at least avoid the crash.
(Reporter)

Updated

6 years ago
Attachment #614972 - Flags: review?(gpascutto)
(Assignee)

Updated

6 years ago
Attachment #614972 - Attachment is patch: true
(Assignee)

Comment 1

6 years ago
Comment on attachment 614972 [details] [diff] [review]
Patch

Review of attachment 614972 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/android/SQLiteBridge.cpp
@@ +289,5 @@
>                  if (isString != JNI_TRUE) {
>                      asprintf(&errorMsg, "Parameter is not of String type");
>                      goto error_close;
>                  }
> +                if (jObjectParam == NULL) {

I don't think this can work, or if it does, it's by coincidence: the lines just above this check IsInstanceOf(jObjectParam, stringclass), which I'd presume return false for NULL (if they don't, that's the "by coincidence" part) and bail out entirely if that's the case. So your code should never be reached.

@@ +295,5 @@
> +                } else {
> +                  jstring jStringParam = (jstring)jObjectParam;
> +                  const char* paramStr = jenv->GetStringUTFChars(jStringParam, NULL);
> +                  LOG("paramStr %s\n", paramStr);
> +                  // SQLite parameters index from 1.

Move this up to the first occurrence of i+1.
Attachment #614972 - Flags: review?(gpascutto) → review-
(Reporter)

Comment 2

6 years ago
(In reply to Gian-Carlo Pascutto (:gcp) from comment #1)
> just above this check IsInstanceOf(jObjectParam, stringclass), which I'd
> presume return false for NULL (if they don't, that's the "by coincidence"
> part)

Hmm... Docs say "Returns JNI_TRUE if obj can be cast to clazz; otherwise, returns JNI_FALSE. A NULL object can be cast to any class."

http://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/functions.html
(Assignee)

Comment 3

6 years ago
You are right. But to explain what I mean with "works by coincidence":

(null instanceof <anything>)        -> false
jenv->IsInstaceOf(NULL, <anything>) -> JNI_TRUE

Does this make sense? Not to me - I'd rather not rely on this behavior, to be honest! But it's guaranteed by the spec, I'll give you that.

I think we can just move the NULL check above the String check and avoid this weirdness.
(Reporter)

Comment 4

6 years ago
Created attachment 615362 [details] [diff] [review]
Path 2

Moved the NULL check before the String check. I hope that's what you meant by "Move this up to the first occurrence of i+1." ?
Assignee: nobody → wjohnston
Attachment #614972 - Attachment is obsolete: true
Attachment #615362 - Flags: review?(gpascutto)
(Assignee)

Updated

6 years ago
Attachment #615362 - Flags: review?(gpascutto) → review+
(Assignee)

Comment 5

6 years ago
>I hope that's what you meant by "Move this up to the first occurrence of i+1." ?

That was actually referring to the comment that explains why it's i+1 instead of just i (" // SQLite parameters index from 1."). The new code also uses i+1 so it's more logical to read if the comment sets next to the first occurrence.
(Reporter)

Updated

6 years ago
blocking-fennec1.0: --- → ?
Can you provide examples of where a select arguments array contains a null? That shouldn't happen.
blocking-fennec1.0: ? → -
(Reporter)

Comment 7

6 years ago
I'm not sure why I never landed this

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e36fd7113ba

I only made this happen when I had some strange things going on in my profile. But I think its good to have the checks around.

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/8e36fd7113ba
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(In reply to Wesley Johnston (:wesj) from comment #9)
> backed this out for crashing
> https://hg.mozilla.org/integration/mozilla-inbound/rev/edb9750342f7

What were the crashes? Is there a bug for that? (Not mentioned in the backout.)
(Reporter)

Comment 11

6 years ago
Whoops. Meant to link things over: bug 799141.
Blocks: 799141
(Reporter)

Comment 12

6 years ago
Comment on attachment 615362 [details] [diff] [review]
Path 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): I'd like to request this be backed out of Aurora. I'm fairly sure it caused bug 799141.
User impact if declined: Crashes on Aurora for users using Sync.
Testing completed (on m-c, etc.): Backed out of central today. Tested locally and the crashes went away.
Risk to taking this patch (and alternatives if risky): Low risk. Patch wasn't necessary for any reason I know of.
String or UUID changes made by this patch: None.
Attachment #615362 - Flags: approval-mozilla-aurora?

Updated

6 years ago
No longer blocks: 799141
Depends on: 799141
(Assignee)

Comment 13

6 years ago
Comment on attachment 615362 [details] [diff] [review]
Path 2

Review of attachment 615362 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/android/SQLiteBridge.cpp
@@ +284,5 @@
>              for (int i = 0; i < numPars; i++) {
>                  jobject jObjectParam = jenv->GetObjectArrayElement(jParams, i);
> +                if (jObjectParam == NULL) {
> +                  rc = f_sqlite3_bind_null(ppStmt, i + 1);
> +                  continue;

We should've put a check for the rc value here.

Updated

6 years ago
Target Milestone: Firefox 18 → Firefox 19

Comment 15

6 years ago
(In reply to Ed Morley [:edmorley UTC+1] from comment #14)
> https://hg.mozilla.org/mozilla-central/rev/edb9750342f7
It's a backout.

Pushed to Aurora 18: http://hg.mozilla.org/releases/mozilla-aurora/rev/8e36fd7113ba
Status: RESOLVED → REOPENED
status-firefox18: --- → fixed
status-firefox19: --- → affected
Resolution: FIXED → ---
(In reply to Wesley Johnston (:wesj) from comment #12)
> Comment on attachment 615362 [details] [diff] [review]
> Path 2
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): I'd like to request this be backed
> out of Aurora. I'm fairly sure it caused bug 799141.
> User impact if declined: Crashes on Aurora for users using Sync.
> Testing completed (on m-c, etc.): Backed out of central today. Tested
> locally and the crashes went away.
> Risk to taking this patch (and alternatives if risky): Low risk. Patch
> wasn't necessary for any reason I know of.
> String or UUID changes made by this patch: None.

Did you mean to nominate for FF17 approval on beta?
(Reporter)

Comment 17

6 years ago
I landed this on FF18 last week, but somehow its not on aurora that I can see?

https://hg.mozilla.org/releases/mozilla-aurora/filelog/7f77dbd943be/mozglue/android/SQLiteBridge.cpp

Comment 18

6 years ago
(In reply to Wesley Johnston (:wesj) from comment #17)
> I landed this on FF18 last week, but somehow its not on aurora that I can
> see?
Indeed, it's no in Aurora although I don't see the backout in the Aurora pushlog.
status-firefox18: fixed → affected
Comment on attachment 615362 [details] [diff] [review]
Path 2

Looks like this is already backed out on aurora and does not need an aurora approval request .
Attachment #615362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Reporter)

Comment 20

6 years ago
Comment on attachment 615362 [details] [diff] [review]
Path 2

OK. This shows up in hg but not mxr. Sorry about the confusion, but this DOES need to land on Aurora.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): I'd like to request this be backed out of Aurora. I'm fairly sure it caused bug 799141.
User impact if declined: Crashes on Aurora for users using Sync.
Testing completed (on m-c, etc.): Backed out of central today. Tested locally and the crashes went away.
Risk to taking this patch (and alternatives if risky): Low risk. Patch wasn't necessary for any reason I know of.
String or UUID changes made by this patch: None.
Attachment #615362 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?

Updated

6 years ago
Attachment #615362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 22

6 years ago
Taking this to figure out why this seemingly correct patch caused so much trouble - it's worrying me a bit.
Assignee: wjohnston → gpascutto
(Assignee)

Updated

6 years ago
Blocks: 752772
(Assignee)

Comment 23

6 years ago
FWIW I can't reproduce the crashes, and adding some code which exercises this code path works as expected.
(Assignee)

Comment 24

6 years ago
I'm not even able to reproduce this on 8e36fd7113ba any more.

Maybe we can consider re-landing this (with the extra return value check)? As far as we can tell the crashes were caused by another error in the code. We probably do want to find that one... The Aurora merge is further away now, so we have some weeks before it would hit more users.

Wesj, what do you think? Anyone else want to chime in?
(Assignee)

Updated

6 years ago
Flags: needinfo?
(Reporter)

Comment 25

6 years ago
I also can't reproduce. Relanded this (with the rc check). Will grab an inbound build as soon as one is available and test it:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e28405983f87
Flags: needinfo?
https://hg.mozilla.org/mozilla-central/rev/e28405983f87
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED

Updated

6 years ago
status-firefox19: affected → ---

Updated

6 years ago
Depends on: 805432
Backed out to fix bug 805432
https://hg.mozilla.org/mozilla-central/rev/972986c4f75e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

6 years ago
So, *this* patch is causing *NSSBridge* to fail? That's not good. 

I hope someone took a backtrace before backing out this time :-/
You need to log in before you can comment on or make changes to this bug.