738347 - SQLiteBridge does not check for nulls in ContentValues

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

while(valueIterator.hasNext()) {
            Entry<String, Object> value = valueIterator.next();
            keyNames.add(value.getKey());
            valueNames.add("?");
            valueBinds.add(value.getValue().toString());
        }

will NPE if you do:

  values.putNull("foo");

which is the only way to get an explicit null in an INSERT.

(Also, the code for building up "?, ?" strings is really inefficient.)

Comment 1

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

Richard - Please re-nom if this is blocker-worthy

Comment 2

[Allison Naaktgeboren :ally]

eg: does this cause anything to blow up, or is this a 'nice to have, good hygiene' sort of thing?

Comment 3

[Richard Newman [:rnewman]]

It doesn't cause any problems that we can't work around (in this case, the table defaults to NULL for that column), but this spikes the ability to use SQLiteBridge for tables with columns that don't default to NULL.

Requires some upstream defensive programming, but not a beta blocker.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch adds null checks to "update" part of SQLiteBridge.

Comment 5

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

Comment on attachment 611624 [details] [diff] [review]
Patch

>         sb.append(" SET ");
>         while(valueIterator.hasNext()) {
>             Entry<String, Object> value = valueIterator.next();
>             sb.append(value.getKey() + " = ?");

Does sqlite handle "Xxx = NULL" or does it want "Xxx IS NULL" ?

Can the string concatenation be replaced with:

              sb.append(value.getKey());
              sb.append(value.getValue() == null ? " IS ?" : " = ?");

Let's figure out the "IS NULL" issue. Also, let's get Richard to give a thumbs up too

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 611624 [details] [diff] [review]
Patch

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

You *definitely* need to add tests for this. Because this patch would fail them :D

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +164,5 @@
>          sb.append(" SET ");
>          while(valueIterator.hasNext()) {
>              Entry<String, Object> value = valueIterator.next();
>              sb.append(value.getKey() + " = ?");
> +            valueNames.add(value.getValue() == null ? "NULL" : value.getValue().toString());

This won't work. It'll add the string "NULL" to the database, not a NULL.

You might be able to do something like

  sb.append(value.getKey());
  Object v = value.getValue();
  if (v == null) {
    sb.append(" = NULL");
  } else {
    sb.append(" = ?");
    valueNames.add(v.toString());
  }

but you might need to adjust some counts somewhere.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #5)

> Does sqlite handle "Xxx = NULL" or does it want "Xxx IS NULL" ?

In UPDATE, the production is "SET column_name = expr", and NULL is an expr.

http://sqlite.org/lang_update.html

IS NULL is for WHERE clauses, because nothing is = to NULL.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Added null checks and avoided binding "NULL" (as a string) in this patch. It's been long since I worked on SQL (like 8 years probably) and this is not my area of expertise. I'm still not sure if this is the needed fix. :(

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 611857 [details] [diff] [review]
Patch

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

A few adjustments, but most importantly you *need* tests for this. Doesn't have to be much; just one UPDATE that sets one column to null for some records, and queries to verify.

Clearing review flags until that.

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +160,3 @@
>          ArrayList<String> valueNames = new ArrayList<String>();
>  
> +        StringBuilder sb = new StringBuilder("UPDATE " + table + " SET ");

More efficient to use multiple appends here. You're concatenating a string then passing it to the StringBuilder.

@@ +160,4 @@
>          ArrayList<String> valueNames = new ArrayList<String>();
>  
> +        StringBuilder sb = new StringBuilder("UPDATE " + table + " SET ");
> +        boolean hasMore = false;

I think naming this 'isFirst' and defaulting to true would be clearer...

  is (isFirst)
    isFirst = false;
  else
    sb.append(", ");

@@ +174,5 @@
> +            if (val == null) {
> +                sb.append("NULL");
> +            } else {
> +                sb.append("?");
> +                valueNames.add(val.toString());

More efficient to do what I mentioned in my comment:

  sb.append(value.getKey());
  Object val = value.getValue();
  if (val == null) {
    sb.append(" = NULL");
  } else {
    sb.append(" = ?");
    valueNames.add(val.toString());
  }

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I have made changes, avoiding string concatenations (Aah! I read a blog on why StringBuilder is better, but didn't quite apply it! :sigh: ).

How do I test this? Some pointers will be helpful.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)

> How do I test this? Some pointers will be helpful.

You can probably figure something out from

https://wiki.mozilla.org/Auto-tools/Projects/Robocop/WritingTests#Creating_a_new_Content_Provider_test

-- this isn't a Content Provider test, but it sure sets up some databases and such. You could also write tests for a Content Provider that uses the SQLiteBridge, and call UPDATE through the CP with a null in the ContentValues object.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 611868 [details] [diff] [review]
Patch

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

Looks good, please re-r? when tests are ready.

Comment 13

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/mozilla-central/file/d698e656b1e0/mobile/android/base/tests/testBrowserProvider.java.in#l360 <-- Seems like there are tests that does this already. Are we good then?

Comment 14

[Wesley Johnston (:wesj)]

Those tests do not test this code path. They are for bookmarks and history, which don't use Gecko's sqlite. We do have some really simple tests for these two providers. Should be easy to modify them to test this. see testPasswordProvider and testFormHistory.

Comment 15

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Tests to add NULL in ContentValues in SQLiteBridge

This changes the tests to have NULL values. Also, fixes CursorMatches() to avoid NPEs.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Add NULL checks in ContentValues in SQLiteBridge

This patch fixes handling NULL checks in insert() and update() part of SQLiteBridge.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 612647 [details] [diff] [review]
Patch: Tests to add NULL in ContentValues in SQLiteBridge

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

f+, not r+, because I'd like to see a specific case of an UPDATE nulling out a non-null value. That is:

* Insert a non-null value
* Check it's not null
* Update with a null
* Check it's null.

I believe this test now always uses a null GUID, which is half-way there...

Make sure to *continue* to test the case of INSERT with a null value, too.

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 612648 [details] [diff] [review]
Patch: Add NULL checks in ContentValues in SQLiteBridge

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

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +121,5 @@
>  
>      public long insert(String table, String nullColumnHack, ContentValues values)
>                 throws SQLiteBridgeException {
> +        if (values == null)
> +            return 0;

I might be inclined to throw an IllegalArgumentException here; match what the Android library does?

@@ +143,5 @@
>          StringBuilder sb = new StringBuilder("INSERT into ");
>          sb.append(table);
>  
>          sb.append(" (");
>          sb.append(TextUtils.join(", ", keyNames));

This makes me cry every time I see it.

@@ +160,5 @@
>  
>      public int update(String table, ContentValues values, String whereClause, String[] whereArgs)
>                 throws SQLiteBridgeException {
> +        if (values == null)
> +            return 0;

Same.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Tests to add NULL in ContentValues in SQLiteBridge

This patch changes tests of FormHistory provider to check NULL insertion and NULL updation.
1. insert with NON-NULL value and update it to NULL.
2. insert with NULL value and update it to NON-NULL.

The fix patch posted runs fine without crashing! yaay! :)

Comment 20

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/308fedc536c1
http://hg.mozilla.org/integration/mozilla-inbound/rev/36adc9ed1d0e

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/308fedc536c1
https://hg.mozilla.org/mozilla-central/rev/36adc9ed1d0e