745384 - SQLiteBridge doesn't handle NULL's in query parameters

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: 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.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 2

[Wesley Johnston (:wesj)]

(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

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: 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." ?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

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

Comment 6

[Richard Newman [:rnewman]]

Can you provide examples of where a select arguments array contains a null? That shouldn't happen.

Comment 7

[Wesley Johnston (:wesj)]

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

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e36fd7113ba

Comment 9

[Wesley Johnston (:wesj)]

backed this out for crashing
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb9750342f7

Comment 10

[Richard Newman [:rnewman]]

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

Comment 11

[Wesley Johnston (:wesj)]

Whoops. Meant to link things over: bug 799141.

Comment 12

[Wesley Johnston (:wesj)]

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.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edb9750342f7

Comment 15

[Scoobidiver (away)]

(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

Comment 16

[Alex Keybl [:akeybl]]

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

Comment 17

[Wesley Johnston (:wesj)]

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

[Scoobidiver (away)]

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

Comment 19

[bhavana bajaj [:bajaj]]

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 .

Comment 20

[Wesley Johnston (:wesj)]

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.

Comment 21

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/081331e82efd

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Taking this to figure out why this seemingly correct patch caused so much trouble - it's worrying me a bit.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

FWIW I can't reproduce the crashes, and adding some code which exercises this code path works as expected.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 25

[Wesley Johnston (:wesj)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e28405983f87

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out to fix bug 805432
https://hg.mozilla.org/mozilla-central/rev/972986c4f75e

Comment 28

[Gian-Carlo Pascutto [:gcp]]

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

I hope someone took a backtrace before backing out this time :-/

Comment 29

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.