Fixing null pointer dereference.

Fixing null pointer dereference.

BUG=chromium:158178
TEST=units,make sure such SEGV doesnt happen on x86-generic-bot (where it pops periodically now)


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177399

[glotov, 2013-01-16 11:02:07]

Hi! Please have a look.

[Scott Hess - ex-Googler, 2013-01-16 17:06:18]

This implies that we've received a sqlite error from a statement which was valid
at some point.  As of yet, I don't see a valid path which leads to that case. 
GetUntrackedStatement maybe could - do you have any pointers to the context this
is happening in?

[glotov, 2013-01-16 17:08:58]

Yes, I updated the bug history yesterday. My Cros ASAN bots catch this situation
twice a day.

On 2013/01/16 17:06:18, shess wrote:
> This implies that we've received a sqlite error from a statement which was
valid
> at some point.  As of yet, I don't see a valid path which leads to that case. 
> GetUntrackedStatement maybe could - do you have any pointers to the context
this
> is happening in?

[Scott Hess - ex-Googler, 2013-01-16 17:19:50]

Apologies, brain skip on my part.

Let me look at bit at the code.  AFAICT, this is only a problem if you get
an error in certain internal/utility functions, it may be that a better
solution would be to refactor those functions to not allow that path than
to make that path more relaxed with handling errors.  For other users, it
shouldn't be possible to get to that point with a NULL connection.

-scott


On Wed, Jan 16, 2013 at 9:08 AM, <glotov@chromium.org> wrote:

> Yes, I updated the bug history yesterday. My Cros ASAN bots catch this
> situation
> twice a day.
>
>
> On 2013/01/16 17:06:18, shess wrote:
>
>> This implies that we've received a sqlite error from a statement which was
>>
> valid
>
>> at some point.  As of yet, I don't see a valid path which leads to that
>> case.
>> GetUntrackedStatement maybe could - do you have any pointers to the
>> context
>>
> this
>
>> is happening in?
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/11886065/<https://chromiumcodereview...
>

[Scott Hess - ex-Googler, 2013-01-16 20:49:07]

LGTM, with minor request for a TODO.

I would rather the connection couldn't be NULL, because this means we're missing
out on error signals.  Unfortunately, since GetUntrackedStatement() is const
(for good reasons), it's challenging to reconcile.  I've tried a couple options,
but so far all seem to require an excessive amount of run-on changes.

https://chromiumcodereview.appspot.com/11886065/diff/11003/sql/connection.h
File sql/connection.h (right):

https://chromiumcodereview.appspot.com/11886065/diff/11003/sql/connection.h#n...
sql/connection.h:375: // StatementRef::StatementRef(sqlite3_stmt* stmt).
Please rephrase the change something like:

// TODO(shess): connection_ can be NULL in case of GetUntrackedStatement(),
which prevents Statement::OnError() from forwarding errors.

Mostly, I'd rather aim to restore the guarantee in the future.

[glotov, 2013-01-17 10:58:22]

https://chromiumcodereview.appspot.com/11886065/diff/11003/sql/connection.h
File sql/connection.h (right):

https://chromiumcodereview.appspot.com/11886065/diff/11003/sql/connection.h#n...
sql/connection.h:375: // StatementRef::StatementRef(sqlite3_stmt* stmt).
On 2013/01/16 20:49:07, shess wrote:
> Please rephrase the change something like:
> 
> // TODO(shess): connection_ can be NULL in case of GetUntrackedStatement(),
> which prevents Statement::OnError() from forwarding errors.
> 
> Mostly, I'd rather aim to restore the guarantee in the future.

Done.

[commit-bot: I haz the power, 2013-01-17 10:58:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/glotov@chromium.org/11886065/10008

[commit-bot: I haz the power, 2013-01-17 13:11:19]

Change committed as 177399