Closed Bug 538940 Opened 10 years ago Closed 10 years ago
Misuse of PR
_Get Error Text Length when allocating error message buffer in nsprpub/pr/tests/dlltest .c
All is in the summary. Patch attached.
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+
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
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.