373935 - js_strdup does not check for overflow

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[lsg-mtso]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.2) Gecko/20070225 BonEcho/2.0.0.2
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.2) Gecko/20070225 BonEcho/2.0.0.2

JS_strdup in jsapi.c performs a +1 to the memory allocated with JS_malloc() with out checking for overflow.  The following memcpy could then overwrite the heap causing heap corruption.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Martijn Wargers (dead)]

Reporter, could you file these types of bugs under Core->Javascript Engine? Thanks!

Comment 2

[Brian Crowder]

how does memcpy(....., 0) cause heap corruption?

Comment 3

[Daniel Veditz [:dveditz]]

Although not as straightforward as the cases that allocate 0 and then copy 0xffffffff into it, in this case JS_strdup will return an uninitialized and not null-terminated string back to the caller. Whether that leads to corruption depends on the caller (and the corruption wouldn't happen at strdup's memcpy as stated in comment 0).

Probably it'll just end up with an access violation reading the string, but if something tries to write into the copy they'll go bad. There are too many callers to be worth checking them all to see how bad this is (and it's a public API, so other spidermonkey-using apps could be in trouble, too) so let's just fix it here.

Comment 4

[Brian Crowder]

Attachment: fail with a null return on overflow

dveditz:  Thanks for convincing me there's a bug here.

Comment 5

[Igor Bukanov]

Lets consider the code in question:

 JS_PUBLIC_API(char *)
 JS_strdup(JSContext *cx, const char *s)
 {
     size_t n;
     void *p;
 
     n = strlen(s) + 1;
     p = JS_malloc(cx, n);
     if (!p)
         return NULL;
     return (char *)memcpy(p, s, n);
 }

Here n is the number of bytes in char array pointed by s including the terminating '\0'. According to the definition of size_t, it can not exceed SIZE_T_MAX since SIZE_T_MAX is exactly the maximum size of any object. Thus there is no overflow.

What have I missed?

Comment 6

[Brian Crowder]

Igor:  Are you asserting that strlen() can never return ~(size_t)0?  It would always return (~(size_t)0 - 1) or less?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Igor:  Are you asserting that strlen() can never return ~(size_t)0?  It would
> always return (~(size_t)0 - 1) or less?

I am asserting that one can not construct without triggering undefined behavior a char array with capacity exceeding SIZE_T_MAX. Thus, assuming that strlen does not have bugs, the maximum value it can return is SIZE_T_MAX - 1 as:

strlen(chars) + 1 
  == strchr(str, '\0') - str 
  == number of bytes in char array pointed by str 
  <= SIZE_T_MAX.

Comment 8

[Brian Crowder]

I agree with Igor

Comment 9

[Brian Crowder]

Comment on attachment 264449 [details] [diff] [review]
fail with a null return on overflow

Dropping request for review on bogus patch for bogus bug.