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

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Mike Hommey [:glandium]]

Attachment: patch

All is in the summary. Patch attached.

Comment 1

[Wan-Teh Chang]

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.

Comment 2

[Wan-Teh Chang]

Attachment: 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

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Mike Hommey [:glandium]]

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.