mozStorageStatementWrapper::Initialize should check statement state/validity

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Storage
--
minor
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: asuth, Assigned: drexler)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.

Updated

9 years ago
Whiteboard: [good first bug]
(Assignee)

Comment 1

6 years ago
Created attachment 575073 [details]
Validity checks implementation.

Although this is relatively minor and a bit unnecessary, i decided to send this patch. Feedback most welcome.
Attachment #575073 - Flags: review?(sdwilsh)
(Assignee)

Comment 2

6 years ago
Created attachment 575074 [details] [diff] [review]
Validity checks implementation

Reattached the correct file.
Attachment #575073 - Attachment is obsolete: true
Attachment #575074 - Flags: review?(sdwilsh)
Attachment #575073 - Flags: review?(sdwilsh)

Updated

6 years ago
Assignee: nobody → andrew.quartey
Status: NEW → ASSIGNED
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)
Attachment #575074 - Flags: review?(sdwilsh)
(Assignee)

Comment 4

6 years ago
Created attachment 576042 [details] [diff] [review]
Validity checks implementation v2

Made the necessary adjustments. Thanks for the earlier feedback.
Attachment #575074 - Attachment is obsolete: true
Attachment #576042 - Flags: review?(sdwilsh)
Comment on attachment 576042 [details] [diff] [review]
Validity checks implementation v2

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

r=sdwilsh
Attachment #576042 - Flags: review?(sdwilsh) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c475a7dcaa
Keywords: checkin-needed
Whiteboard: [good first bug] → [inbound]
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]
Whiteboard: [inbound]
(Assignee)

Comment 8

6 years ago
(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);
(Assignee)

Comment 9

6 years ago
Created attachment 578042 [details] [diff] [review]
Validity checks implementation v3

Corrected patch.
Attachment #576042 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #578042 - Flags: review?(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
Attachment #578042 - Flags: review?(sdwilsh) → review+

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/28bdd260c3bc
Keywords: checkin-needed
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 14

6 years ago
Sure. Sorry for jumping the gun.
You need to log in before you can comment on or make changes to this bug.