Closed Bug 495097 Opened 11 years ago Closed 11 years ago

sdb_mapSQLError returns signed int, causing sign extension on 64-bit platforms

Categories

(NSS :: Libraries, defect, P1)

3.12
x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: wtc, Assigned: rrelyea)

References

Details

(Whiteboard: FIPS)

Attachments

(1 file, 1 obsolete file)

Attached patch Proposed patch (obsolete) — Splinter Review
sdb_mapSQLError maps SQLite errors to PKCS #11 errors,
but it is incorrectly declared to return (signed) int.
When its return value is assigned to CK_RV, which is
unsigned long, on 64-bit platforms, sign extension
occurs.

The sign extension causes CKR_NETSCAPE_CERTDB_FAILED
(0xce534351) to become 0xffffffffcd534351.

A place where accurate error reporting is important is
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11load.c&rev=1.25&mark=146,153-158,164#122

If I call NSS_InitReadWrite(sql:/home/wtc/.pki/nssdb)
and the directory doesn't exist, I get the confusing
SEC_ERROR_INVALID_ARGS error because the wrong
error code causes secmod_ModuleInit to call the
softoken again with C_Initialize(NULL), and that
fails with SEC_ERROR_INVALID_ARGS.
Attachment #379924 - Flags: review?(rrelyea)
Attachment #379932 - Flags: review?(rrelyea)
Attachment #379924 - Attachment is obsolete: true
Attachment #379924 - Flags: review?(rrelyea)
Comment on attachment 379932 [details] [diff] [review]
Proposed patch with regression test

Though there should already be a test case for this called db test, it may be better to make sure dbtest checks for the correct SEC_ERROR error code.

Also old off fix check in. We are likely to be doing a respin for FIPS, and I'd like to collect this one with the FIPS respin.
Attachment #379932 - Flags: review?(rrelyea) → review+
Priority: -- → P1
Whiteboard: FIPS [Awaiting Softoken's Thaw]
Version: unspecified → 3.12
Bob, could you check in my patch for me?  I'm not following
the respin for FIPS, so I'm afraid that I may miss the right
time to check this in.  Thanks.

I can move the regression test to dbtest.c later.
Assignee: wtc → rrelyea
To problem. You'ved marked it as fips and assigned it to me... so we are set.

bob
Status: NEW → ASSIGNED
RCS file: /cvsroot/mozilla/security/nss/cmd/tests/baddbdir.c,v
done
Checking in cmd/tests/baddbdir.c;
/cvsroot/mozilla/security/nss/cmd/tests/baddbdir.c,v  <--  baddbdir.c
initial revision: 1.1
done
Checking in cmd/tests/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/tests/manifest.mn,v  <--  manifest.mn
new revision: 1.8; previous revision: 1.7
done
Checking in lib/softoken/sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: FIPS [Awaiting Softoken's Thaw] → FIPS
Duplicate of this bug: 603766
You need to log in before you can comment on or make changes to this bug.