1898074 - Memory leak in create_objects_from_handles()

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[mozillabugs]

create_objects_from_handles() (security/nss/lib/dev/devtoken.c) invokes undefined behavior if the allocation of objects[0] (line 216, below) fails, and leaks memory if the allocation of objects[i] fails, where i >= 1.

The first issue is that if the allocation of objects[0] fails, the test on line 217 causes line 218 to be executed, which underflows the int i, which is undefined behavior according to C++23 (N4950) s.6.8.2 ("Overflow for signed arithmetic yields undefined behavior").

The second issue is that if the allocation of objects[i] fails, where i >= 1, the for loop on lines 218-20 doesn't release objects[0] because the condition on line 218 is > 0.

Code from FIREFOX_126_0_RELEASE:

204: static nssCryptokiObject **
205: create_objects_from_handles(
206:     NSSToken *tok,
207:     nssSession *session,
208:     CK_OBJECT_HANDLE *handles,
209:     PRUint32 numH)
210: {
211:     nssCryptokiObject **objects;
212:     objects = nss_ZNEWARRAY(NULL, nssCryptokiObject *, numH + 1);
213:     if (objects) {
214:         PRInt32 i;
215:         for (i = 0; i < (PRInt32)numH; i++) {
216:             objects[i] = nssCryptokiObject_Create(tok, session, handles[i]);
217:             if (!objects[i]) {
218:                 for (--i; i > 0; --i) {
219:                     nssCryptokiObject_Destroy(objects[i]);
220:                 }
221:                 nss_ZFreeIf(objects);
222:                 objects = NULL;
223:                 break;
224:             }
225:         }
226:     }
227:     return objects;
228: }

For the loop, I suggest:

    for (; i > 0; --i) {
        nssCryptokiObject_Destroy(objects[i-1]);
    }

Comment 1

[John Schanck [:jschanck]]

The leak is an issue. I wouldn't consider it a security bug.

I don't see the undefined behavior. Decrementing a signed int equal to 0 is not an underflow (an underflow would be PRInt32 i = INT_MIN; --i;). Am I missing something?

Comment 2

[mozillabugs]

Ugh. You're correct. This is just a leak. Need coffee. Shall I unhide the bug?

Comment 3

[John Schanck [:jschanck]]

Attachment: Bug 1898074 - memory leak in create_objects_from_handles. r=#nss-reviewers

Comment 4

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/351c4cb0d8b639c3f6132fdd7d7a2cb40fcd89d4