377451 - crash in cvt_s called from PR_vsnprintf

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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);

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed fix, v1

This patch solves the documented problem. 
It also imposes the caller's precision value, if positive non-zero,
on the "(null)" string, when relevant.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed fix, v2, with conditionally compiled strnlen implementation

This patch is the same as before, but with the addition of a 
conditionally compiled static implementation of strnlen.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 7

[Wan-Teh Chang]

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/configure.in.  I'd just inline strnlen.  It's
only three lines.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: v3, strnlen inline

Thanks, Wan-Teh.  I don't know what NSPR would do without your broad
knowledge of the various platforms.

Comment 9

[Wan-Teh Chang]

Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline

r=wtc.

>+	for(s = str; prec && *s; s++, prec-- )
           ^                                ^
Please add a space after 'for' and remove the space
before the closing parenthesis.  Thank you, Nelson!

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline

requesting second review for stable branch

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Julien, please review for 4.6.7

Comment 12

[Julien Pierre]

Comment on attachment 261906 [details] [diff] [review]
v3, strnlen inline

Looks good.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Bug 377451.  Fix crash in cvt_s when string is longer than precision. 
r=wtc,julien.pierre

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

On NSPR_4_6_BRANCH:
Checking in prprf.c; new revision: 3.18.2.1; previous revision: 3.18

Wan-Teh, should I also commit this on the NSPRPUB_PRE_4_2_CLIENT_BRANCH ?