524787 - crash [@mozStorageResultSet::GetNextRow(mozIStorageRow**) ]

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

Only seeing one crash here, but this just needs a simple null check on the return value.  NS_ENSURE_ARG_POINTER anyone?

Comment 1

[Shawn Wilsher :sdwilsh]

I suppose it's also possible for GetNextRow to fail here by getting null out of the mData array.  Although, I don't see how that could be null.

Comment 2

[Shawn Wilsher :sdwilsh]

FWIW, checking the argument for non-nullness is what we should do.  If we continue to see crashes with this signature, we'll know it's the later (and be confused).

Comment 3

[David Dahl :ddahl]

Are you looking for something like this, or is there a macro I should be using?

NS_IMETHODIMP
ResultSet::GetNextRow(mozIStorageRow **_row)
{
  if(!*_row){
    // Return an error?
    return NS_OK;
  }

  if (mCurrentIndex >= mData.Count()) {
    // Just return null here
    return NS_OK;
  }

  NS_ADDREF(*_row = mData.ObjectAt(mCurrentIndex++));
  return NS_OK;
}

Comment 4

[Shawn Wilsher :sdwilsh]

I hinted at the macro in comment 0 ;)

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)
> I hinted at the macro in comment 0 ;)
So I tried:

NS_ENSURE_ARG_POINTER(*_row);

and I see this warning in the test suite:

WARNING: NS_ENSURE_TRUE(*_row) failed: file /home/ddahl/code/moz/mozilla-central/mozilla/storage/src/mozStorageResultSet.cpp, line 80


then a test failure:

TEST-UNEXPECTED-FAIL | /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_statement_executeAsync.js | 0 == 11 - See following stack:
JS frame :: /home/ddahl/code/moz/mozilla-central/mozilla/testing/xpcshell/head.js :: do_throw :: line 197
JS frame :: /home/ddahl/code/moz/mozilla-central/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 227
JS frame :: /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/_tests/xpcshell/test_storage/unit/test_statement_executeAsync.js :: anonymous :: line 996

Comment 6

[David Dahl :ddahl]

Attachment: v 0.1 WIP - tests fail

the above mentioned tests fail with this patch:(

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> NS_ENSURE_ARG_POINTER(*_row);
You do not want to dereference _row.  That is what we are trying to protect against crashing on.

Comment 8

[David Dahl :ddahl]

Attachment: v 0.2 All tests pass

Is a test required as well, or is this code path tested well enough?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Is a test required as well, or is this code path tested well enough?
Can't really write a test for this unless we write a native code test.  It's just a null check, and I'm happy with this.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

r=sdwilsh

Would like to see this on branches.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/4f972fa92cd0

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 13

[Johnathan Nightingale [:johnath]]

Comment on attachment 408882 [details] [diff] [review]
v 0.2 All tests pass

Approved for 1.9.2 - simple hygiene. Don't think we'd hold the release for this bug though, so I'll blocking- this momentarily.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9cdb04347c82

Comment 15

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f9e848c13288