631423 - crash [@ mozilla::places::GenerateRandomBytes]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

I've found only 1 report for this in the last 2 weeks, but I think it could make sense

275   BOOL rc = CryptAcquireContext(&cryptoProvider, 0, 0, PROV_RSA_FULL,
276                                 CRYPT_VERIFYCONTEXT | CRYPT_SILENT);
277   if (rc) {
278     rc = CryptGenRandom(cryptoProvider, aSize, _buffer);
279   }
280   (void)CryptReleaseContext(cryptoProvider, 0);

if CryptAcquireContext fails there is no valid assumption on cryptoProvider, we should not try to call CryptReleaseContext or at least null check cryptoProvider.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> if CryptAcquireContext fails there is no valid assumption on cryptoProvider, we
> should not try to call CryptReleaseContext or at least null check
> cryptoProvider.
Except that CryptReleaseContext can handle NULL (it'd just return ERROR_INVALID_HANDLE).  We could certain move it, but it's not going to change anything here.

Comment 2

[Shawn Wilsher :sdwilsh]

Although we don't initialize the value, so we could crash there.  Yes, we should fix this (by either initializing the value, or moving it inside the if.

Comment 3

[Marco Bonardo [:mak]]

well, I don't know what CryptAcquireContext does when it fails, so it's even possible that we try to Release a half baked context regardless what we init. I'd prefer moving it inside the if.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/fa598dd31ac6