493912 - sqlite3_reset should be invoked in sdb_FindObjectsInit when error occurs

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch

Comment 1

[Robert Relyea]

Before I r+ this, could you give a little more context.

Why should it be called (preferably a pointer to the sqlite documentation), and what problems are caused by not calling it.

Don't assume that I don't agree with this patch, it's just the bug has almost no information for me to evaluate it (or tell if this is an isolated incident, or if there are other places that need to be changed as well).

Note that softoken is currently frozen for FIPS validation. Even if I r+ this change it's unlikely to be included in any NSS release for a very long time unless there's a pressing issue it resolves. 

Thanks,

bob

Comment 2

[Hiroyuki Ikezoe (:hiro)]

You have sharp eyes. Actually I have never faced any issues caused by this. I noticed sqlite3_reset is missing in the function when I just read code. So I am not sure what is happened without the line.

Comment 3

[Robert Relyea]

Comment on attachment 378531 [details] [diff] [review]
A patch

OK, I see it's the only place were we don't reset before finalize. I'm guessing that the sqlite docs say we should do this, which is why I reset before all the other finalizes, so even though it doesn't appear to cause any problems, its better to be friendly to sqlite.

bob

Comment 4

[Robert Relyea]

Taking since Hiroyuki probably does not have NSS commit access.

bob

Comment 5

[Robert Relyea]

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.14; previous revision: 1.13
done

Comment 6

[Julien Pierre]

Bob,

The patch that you checked in is not the one that was reviewed.
The code in version 1.14 of sdb.c looks like :

    if (findstmt) {
        sqlite3_reset(stmt);
        sqlite3_finalize(findstmt);
    }

and there is a build error because stmt is undefined.
I have fixed the build by changing smt to findstmt, as the reviewed patch did.

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.15; previous revision: 1.14
done

Comment 7

[Robert Relyea]

Thanks Julien. I thought I had built the patch to make sure I had it correct. obviously not.

bob