Closed Bug 377451 Opened 14 years ago Closed 14 years ago

crash in cvt_s called from PR_vsnprintf


(NSPR :: NSPR, defect, P2)



(Not tracked)



(Reporter: nelson, Assigned: nelson)


(Keywords: crash)


(1 file, 2 obsolete files)

purify reports uninitialized memory reads when we call PR_vsnprintf and pass a string which is not null-terminated, along with the length of string we want printed. Eg if we call  PR_sprintf(buf, "%.*s", 5, "abcdef") then the character 'f' is read when it should not be. 

This causes a crash if we pass a string which is part of a memory mapped file and there were no null bytes past the end of the string to stop the scanning 
(it keeps reading until it tries to access past the end of the file, at which point it crashes).

The problem is in cvt_s. It looks for a null terminator in the string using strlen, *then* uses the precision passed by the caller to override the length 
if the precision is smaller.  Instead it should look for a null within the precision and override the precision with the lower strlen if found. 

Possible crash aside, it's also a waste of cpu cycles to read irrelevant parts of the input string.

      UMR: Uninitialized memory read (7180 times)
      This is occurring while in thread 8:
            strlen         [rtlib.o]
            cvt_s          [prprf.c:386]
                       return 0;
                   /* Limit string length by precision value */
            =>     slen = s ? strlen(s) : 6;
                   if (prec > 0) {
                       if (prec < slen) {
                           slen = prec;
            dosprintf      [prprf.c:988]
                         case 's':
                           u.s = va_arg(ap, const char*);
            =>             rv = cvt_s(ss, u.s, width, prec, flags);
                           if (rv < 0) {
                               return rv;
            PR_vsnprintf   [prprf.c:1197]
                   ss.base = out;
                   ss.cur = out;
                   ss.maxlen = outlen;
            =>     (void) dosprintf(&ss, fmt, ap);
                   /* If we added chars, and we didn't append a null, do it 
                   if( (ss.cur != ss.base) && (*(ss.cur - 1) != '\0') )
            nslogv         [nslogger.c:591]
            domainmap_debug_routine [autocreate.c:131]
            dm_find_hash   [domain_map.c:476]
                           (context, "  %d is hash chain value for %.*s\n", 
            =>              localsymbol.length, localsymbol.body);
Giving credit where it is due:  
This report came from Mr. Fred Batty of Sun Microsystems on August 22, 2003.
Sadly, the report was not turned into a bugzilla bug until now. 
I came across Fred's report just today.  
I'm sorry it took so long, and I'm glad it is now properly filed.

This bug may become a candidate to be a P1.
Priority: -- → P2
Target Milestone: --- → 4.6.6
Attached patch proposed fix, v1 (obsolete) — Splinter Review
This patch solves the documented problem. 
It also imposes the caller's precision value, if positive non-zero,
on the "(null)" string, when relevant.
Assignee: neil.williams → nelson
Attachment #261747 - Flags: review?(wtc)
Comment on attachment 261747 [details] [diff] [review]
proposed fix, v1

> Index: pr/src/io/prprf.c
>     if (prec > 0) {
>+	slen = strnlen(s, prec);
>+    } else {
>+	slen = strlen(s);
>     }

Rats!  Although MSDN doc says strnlen is compatible with Win95 and Win98,
strnlen() is not implemented in MSVC 6's RTL.

I guess we could call PL_strnlen instead.  But I think that the nspr4 shared 
lib is not allowed to have dependencies on the plc4 shared lib.  It doesn't
appear to have any such dependencies now.  

Wan-Teh, any other suggestions?
Nelson, just inline PL_strnlen in this function.
The plc4 shared library is at a higher layer than the
nspr4 shared library.

When you look at MSDN documentation for C run-time library
functions, you also need to check the version of MSVC.
This patch is the same as before, but with the addition of a 
conditionally compiled static implementation of strnlen.
Attachment #261747 - Attachment is obsolete: true
Attachment #261889 - Flags: review?
Attachment #261747 - Flags: review?(wtc)
I should add that I am not certain what version of _MSC_VER to test for.
I know that strnlen is present in MSVC 2005, and that it is NOT present
in MSVC 6.  I don't know about MSVC 7 or MSVC 2003.NET
Attachment #261889 - Flags: review? → review?(wtc)
Comment on attachment 261889 [details] [diff] [review]
proposed fix, v2, with conditionally compiled strnlen implementation

strnlen is a common nonstandard C library extension.  It is
not available everywhere.  It is not in Single Unix Specification,
Version 3, and it is not available on Mac OS X.  So I'm afraid that
this patch needs work.

The proper fix is to add an autoconf feature test for strnlen
in mozilla/nsprpub/  I'd just inline strnlen.  It's
only three lines.
Attachment #261889 - Flags: review?(wtc) → review-
Thanks, Wan-Teh.  I don't know what NSPR would do without your broad
knowledge of the various platforms.
Attachment #261889 - Attachment is obsolete: true
Attachment #261906 - Flags: review?(wtc)
Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline


>+	for(s = str; prec && *s; s++, prec-- )
           ^                                ^
Please add a space after 'for' and remove the space
before the closing parenthesis.  Thank you, Nelson!
Attachment #261906 - Flags: review?(wtc) → review+
Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline

requesting second review for stable branch
Attachment #261906 - Flags: review?(julien.pierre.boogz)
Julien, please review for 4.6.7
Target Milestone: 4.6.6 → 4.6.7
Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline

Looks good.
Attachment #261906 - Flags: review?(julien.pierre.boogz) → review+
Bug 377451.  Fix crash in cvt_s when string is longer than precision. 

On trunk:
Checking in prprf.c; new revision: 3.19; previous revision: 3.18

Checking in prprf.c; new revision:; previous revision: 3.18

Wan-Teh, should I also commit this on the NSPRPUB_PRE_4_2_CLIENT_BRANCH ?
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.