465299 - mozStorageStatementWrapper::Initialize should check statement state/validity

Status: RESOLVED FIXED

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

Description

[Andrew Sutherland [:asuth] (he/him)]

As mentioned on bug 463907 comment 1, mozStorageStatementWrapper doesn't verify that the statement is valid and doesn't check that GetParameterCount and GetColumnCount succeed.  Since both of those getters don't set the value on failure, and the values are not initialized to 0, this could result in excessive iteration and corresponding memory allocation.

I expect the right thing to do is for Initialize to return an error if the statement's state is MOZ_STORAGE_STATEMENT_INVALID.

Marking this as minor as the explicit wrapper class is deprecated and this use case is pretty unlikely too.

Comment 1

[Andrew Quartey [:drexler]]

Attachment: Validity checks implementation.

Although this is relatively minor and a bit unnecessary, i decided to send this patch. Feedback most welcome.

Comment 2

[Andrew Quartey [:drexler]]

Attachment: Validity checks implementation

Reattached the correct file.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 575074 [details] [diff] [review]
Validity checks implementation

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

Thanks for doing this!

I should note that Mozilla prefers spaces instead of tabs, which you are using in your patch.  This looks good once you address my comments :)

::: storage/src/mozStorageStatementWrapper.cpp
@@ +79,5 @@
>    NS_ENSURE_ARG_POINTER(aStatement);
>  
>    mStatement = static_cast<Statement *>(aStatement);
> +	if (!mStatement)
> +		return NS_ERROR_FAILURE;

This is unnecessary since `aStatement` is checked for null by `NS_ENSURE_ARG_POINTER`

@@ +82,5 @@
> +	if (!mStatement)
> +		return NS_ERROR_FAILURE;
> +
> +	PRInt32 state;
> +	mStatement->GetState(&state);

nit: cast this result to `(void)` since we aren't checking it (there is no need; the method always succeeds).

@@ +84,5 @@
> +
> +	PRInt32 state;
> +	mStatement->GetState(&state);
> +	if (state == mozIStorageStatement::MOZ_STORAGE_STATEMENT_INVALID)
> +		return NS_ERROR_FAILURE;

You can just `NS_ENSURE_TRUE` this instead of having an if.  Having the logging that it failed here for that reason is useful (shouldn't happen; indicates a bug in calling code).

@@ +90,5 @@
>    // fetch various things we care about
> +  nsresult rv1 = mStatement->GetParameterCount(&mParamCount);
> +  nsresult rv2 = mStatement->GetColumnCount(&mResultColumnCount);
> +	if (NS_FAILED(rv1) || NS_FAILED(rv2))
> +		return NS_ERROR_FAILURE;

These are known to not fail, hence the reason we don't check them now (and the explicit cast to void)

Comment 4

[Andrew Quartey [:drexler]]

Attachment: Validity checks implementation v2

Made the necessary adjustments. Thanks for the earlier feedback.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 576042 [details] [diff] [review]
Validity checks implementation v2

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

r=sdwilsh

Comment 6

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c475a7dcaa

Comment 7

[Marco Bonardo [:mak]]

sorry, I had to backout since some of the Storage xpcshell-tests need adjustements for this change

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-365166.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageStatementWrapper.initialize]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-365166.js :: test :: line 29" data: no]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-429521.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageStatementWrapper.initialize]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-429521.js :: createStatementWrapper :: line 44" data: no]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-444233.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageStatementWrapper.initialize]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_bug-444233.js :: <TOP_LEVEL> :: line 46" data: no]
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_storage_statement_wrapper.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageStatementWrapper.initialize]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/storage/test/unit/test_storage_statement_wrapper.js :: <TOP_LEVEL> :: line 55" data: no]

Comment 8

[Andrew Quartey [:drexler]]

(In reply to Shawn Wilsher :sdwilsh from comment #5)
> Comment on attachment 576042 [details] [diff] [review] [diff] [details] [review]
> Validity checks implementation v2
> 
> Review of attachment 576042 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r=sdwilsh

Looking back at the error messages this generated in the tests, I think i made a horrendous mistake. Instead of:
NS_ENSURE_TRUE(state == mozIStorageStatement::MOZ_STORAGE_STATEMENT_INVALID, NS_ERROR_FAILURE);

Rather should have been:
NS_ENSURE_TRUE(state != mozIStorageStatement::MOZ_STORAGE_STATEMENT_INVALID, NS_ERROR_FAILURE);

Comment 9

[Andrew Quartey [:drexler]]

Attachment: Validity checks implementation v3

Corrected patch.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 578042 [details] [diff] [review]
Validity checks implementation v3

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

(In reply to andrew (:drexler) from comment #8)
> Looking back at the error messages this generated in the tests, I think i
> made a horrendous mistake. Instead of:
Don't worry; I should have caught that in review too.  Sometimes the simplest of patches contain the most obvious errors :)

This is why we have tests.

r=sdwilsh

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/28bdd260c3bc

Comment 12

[Marco Bonardo [:mak]]

We don't mark bugs as RESOLVED FIXED till they are merged to the mozilla-central branch. In this case I'm not reopening since it's going to happen in minutes, but please mind that in future.

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/28bdd260c3bc

Comment 14

[Andrew Quartey [:drexler]]

Sure. Sorry for jumping the gun.