Misuse of PR_GetErrorTextLength when allocating error message buffer in nsprpub/pr/tests/dlltest.c

RESOLVED FIXED in 4.8.4

Status

NSPR
NSPR
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: glandium, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.14 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
Created attachment 421020 [details] [diff] [review]
patch

All is in the summary. Patch attached.
(Reporter)

Updated

8 years ago
Attachment #421020 - Attachment is patch: true
Attachment #421020 - Attachment mime type: application/octet-stream → text/plain
Attachment #421020 - Flags: review?(wtc)
(Assignee)

Comment 1

8 years ago
Comment on attachment 421020 [details] [diff] [review]
patch

r=wtc.

Thanks for the bug report and the patch.  I've had a lot
of pain with PR_GetErrorTextLength because the implementation
doesn't match its specification, and it's not clear which
one I should fix.

PR_GetErrorTextLength was originally designed to be
very flexible -- it doesn't assume the error text is
null-terminated, and it doesn't assume the error text
is single-byte.  For example, the error text could
be a UTF-16 string, terminated by a two-byte 0.  The
value returned by PR_GetErrorTextLength is supposed
to include any null character (1 or 2-byte) at the end.

But I remember we changed the implementation so that
it assumes the string is null-terminated and the
value returned by PR_GetErrorTextLength does not
include the terminating null byte.

I'm inclined to change the documentation of R_GetErrorTextLength
to match the current implementation.

In any case, this patch is correct and necessary for
the current implementation.
Attachment #421020 - Flags: review?(wtc) → review+
(Assignee)

Comment 2

8 years ago
Created attachment 421748 [details] [diff] [review]
patch v2

I found that PR_GetErrorText does nothing if the current
error text length is 0 -- it doesn't copy the terminating
null byte into the output buffer.  So the buffer is not
null-terminated in that case.

I could change PR_GetErrorText to write a null byte to
the output buffer in that case, but I'm afraid that'll
break existing code.  So I'm going to have the caller
ensure that the output buffer is null-terminated.  This
patch is a simple way to do that.

Mike, could you please test this patch?  If it works,
I'll apply this fix to your NSS patch in bug 538939.

In the interest of time, I checked in this patch on the
NSPR trunk (NSPR 4.8.4).

Checking in dlltest.c;
/cvsroot/mozilla/nsprpub/pr/tests/dlltest.c,v  <--  dlltest.c
new revision: 3.10; previous revision: 3.9
done
Attachment #421020 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #421748 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

8 years ago
Comment on attachment 421748 [details] [diff] [review]
patch v2

Another simple fix is to change PR_MALLOC to PR_CALLOC.
It does more work than necessary because PR_GetErrorText
does null-terminate the output buffer when the error
text length is not 0.
(Assignee)

Updated

8 years ago
Target Milestone: --- → 4.8.4
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 4

8 years ago
Comment on attachment 421748 [details] [diff] [review]
patch v2

(In reply to comment #3)
> (From update of attachment 421748 [details] [diff] [review])
> Another simple fix is to change PR_MALLOC to PR_CALLOC.
> It does more work than necessary because PR_GetErrorText
> does null-terminate the output buffer when the error
> text length is not 0.

And error cases are seldom. I like this idea.
Attachment #421748 - Flags: review?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.