Closed Bug 739469 Opened 8 years ago Closed 8 years ago

Off-by-one error in PR_GetUniqueIdentity() when >16 identities are requested

Categories

(NSPR :: NSPR, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ekr, Assigned: wtc)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch provided by WTC (obsolete) — Splinter Review
In the initial invocation of this function, the identity cache is
empty, so a cache size of ID_CACHE_INCREMENT (16) is allocated:

    if (length < (identity_cache.ident + 1))
    {
        length += ID_CACHE_INCREMENT;
        names = (char**)PR_CALLOC(length * sizeof(char*));
        if (NULL == names)
        {
            if (NULL != name) PR_DELETE(name);
            PR_SetError(PR_OUT_OF_MEMORY_ERROR, 0);
            return PR_INVALID_IO_LAYER;
        }
    }

  /* now we get serious about thread safety */
    PR_Lock(identity_cache.ml);
    PR_ASSERT(identity_cache.ident <= identity_cache.length);
    identity = identity_cache.ident + 1;
    if (identity > identity_cache.length)  /* there's no room */
    {       
        if ((NULL != names) && (length >= identity))
        {
            /* what we did is still okay */
            memcpy(
                names, identity_cache.name,
                identity_cache.length * sizeof(char*));
            old = identity_cache.name;
            identity_cache.name = names;
            identity_cache.length = length;
            names = NULL;
        }

      ...

    if (NULL != name) /* there's a name to be stored */
    {
        identity_cache.name[identity] = name;
    }


At this point, identity_cache.length is set to 16 and then
identity.cache.name points to a buffer of sizeof(char *) * 16.
Now, if we come back when identity_cache.ident == 15, then
the test at the top doesn't trigger and so we just keep bubbling
down to where identity = identity_cache.ident + 1 (16)
and then we assign identity_cache.name[16], which is one
past the end of the buffer.
Attachment #609536 - Flags: review?(ekr)
Attachment #609536 - Attachment is patch: true
Comment on attachment 609536 [details] [diff] [review]
Patch provided by WTC

Review of attachment 609536 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/nsprpub/pr/src/io/prlayer.c
@@ +612,5 @@
>      /* this initial code runs unsafe */
>  retry:
>      PR_ASSERT(NULL == names);
>      length = identity_cache.length;
> +    if ((identity_cache.ident + 1) >= length)

So to confirm that I understand this reasoning:

in the initial round, identity_cache.ident == 0 && identity_cache.length ==0,
so the > part of the assertion is true. In later rounds, identity_cache.ident
is always at least 1 less than length, so the equality part is true?

If so I wonder if the logic might be clearer if we did:
if ((identity_cache.length == 0) || (identity_cache.length == (identity_cache.ident +1))

Actually, that's not so clear, so maybe a comment would be more helpful.
Attachment #609536 - Flags: review?(ekr) → review+
Re: if ((identity_cache.ident + 1) >= length)

identity_cache.ident + 1 is the index that we will add to
the identity_cache.name array, so we need to make sure
it is with the array bound.  This means
identity_cache.ident + 1 should be < length.
So if identity_cache.ident + 1 is >= length, we know
we need to grow the identity_cache.name array.

Also, this function was written to avoid calling malloc
while holding the lock, so it needs the complicated
doublechecking and retry code.
Right. I'm just trying to figure out if there is any way that that identity_cache.ident +1 can be > than length
except at the start? If so, that would indicate we had already overrun right?
I see.  You are right.  Indeed the only way that
identity_cache.ident +1 can be > than length is
at the start.
I added a comment as Ekr suggested.

Patch checked in on the NSPR trunk (NSPR 4.9.1).

Checking in prlayer.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlayer.c,v  <--  prlayer.c
new revision: 3.21; previous revision: 3.20
done
Attachment #609536 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 8 years ago
Priority: -- → P2
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 4.9.1
You need to log in before you can comment on or make changes to this bug.