Closed Bug 413735 Opened 12 years ago Closed 11 years ago

JS shell, js.c has a buffer overflow for user-supplied input

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: wes, Assigned: mrbkap)

References

Details

(Keywords: verified1.9.0.15, verified1.9.1, Whiteboard: fixed-in-tracemonkey [sg:nse] local critical bug only if js shell present)

Attachments

(5 files, 11 obsolete files)

8.01 KB, patch
igor
: review+
Details | Diff | Splinter Review
14.93 KB, patch
igor
: review+
Details | Diff | Splinter Review
8.02 KB, patch
Details | Diff | Splinter Review
3.25 KB, text/plain
Details
6.46 KB, application/x-javascript
Details
The loop which consumes user-supplied input in interactive mode does not check input bounds.

If any input line, or series of input lines which consist of a single compilable unit, exceed 4095 bytes in length, we blow the 4096-byte buffer "buffer" which is allocated on the stack in Process() (~line 228 in trunk).

* crowder believes some OSes ship with the JS shell compiled.

To Replicate: create a valid javascript statement more than 4096 bytes long (such as a very long array declaration), and feed it to the js shell in interactive mode, thus:

# ./js -i < long_statement.js
Whiteboard: DUPEME
Asking Andrei to confirm/fix.
Assignee: wes → andrei
Straight forward buffer overflow mitigated by a very specific attack vector.  Marking [sg:high] (i.e. sg:critical with significant mitigations).
Whiteboard: DUPEME → DUPEME [sg:high]
Lucas: FYI in case not known, js.cpp is not built into Firefox or any Mozilla app. It's just the testing REPL or "js shell". May be distributed on some BSDs though.

/be
Attached patch Proposed patch (obsolete) — Splinter Review
It was assumed that the buffer to load compilation unit should have fixed size of 4096 chars. If compilation unit was bigger than 4096 it went out of the buffer limits overwriting the memory allocated after the buffer. Of course, it caused crashes of the shell even for simple tests. The proposed patch checks the buffer size and reallocates it if it goes to the limit. If further memory allocation is not possible the patch reports out of memory and returns.
Attachment #348965 - Flags: review?(igor)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #348965 - Attachment is obsolete: true
Attachment #348971 - Flags: review?(igor)
Attachment #348965 - Flags: review?(igor)
In Process(), when do you release "buffer"?
Also, why the additional newlines in DisassFile?
Comment on attachment 348971 [details] [diff] [review]
Updated patch

> static JSBool
>-GetLine(JSContext *cx, char *bufp, FILE *file, const char *prompt) {
>+GetLine(JSContext *cx, char *bufp, size_t bufferSize, FILE *file, const char *prompt) {

The code should continue to work with EDITLINE defined. I think a simplest solution would be for GetLine when EDITLINE is not defined is to do the same thing that readline does. That is, it should allocate the buffer and return it after reading the whole line. 

> static void
> Process(JSContext *cx, JSObject *obj, char *filename, JSBool forceTTY)
> {
...
>+                buffer = (char *) realloc(buffer, bufferSize * sizeof(char));
>+                if (!buffer) {
>+                    JS_ReportOutOfMemory(cx);

This would not release the original value of buffer when realloc fails.
Attachment #348971 - Flags: review?(igor)
Attached patch Updated patch (obsolete) — Splinter Review
Updated patch. Tested for both EDITLINE defined and undefined.
Attachment #348971 - Attachment is obsolete: true
Attachment #349747 - Flags: review?(igor)
Attached patch Optimized patch (obsolete) — Splinter Review
Memory allocation and buffer size strategy are optimized according to Igor's comments
Attachment #349747 - Attachment is obsolete: true
Attachment #349813 - Flags: review?(igor)
Attachment #349747 - Flags: review?(igor)
Comment on attachment 349813 [details] [diff] [review]
Optimized patch

>+static AddLineState
>+AddLine(JSContext *cx,
>+        ResizableCharBuffer **pointerToPointer,
>+        FILE *file,
>+        const char *prompt) {

Drive-by: opening brace of function body in column one on its own line, always.

>+        if ( newBufferSize < requiredSize) {
>+            newBufferSize = requiredSize;
>+        }

Another nit: don't over-brace single-line consequent of single-line if (condition).

Just a couple of style nits, not meant to preempt igor's review.

/be
Attached patch Updated patch (obsolete) — Splinter Review
A couple fixes of style according to Brendan's comments
Attachment #349813 - Attachment is obsolete: true
Attachment #349828 - Flags: review?(igor)
Attachment #349813 - Flags: review?(igor)
Attachment #349828 - Flags: review?(igor) → review-
Comment on attachment 349828 [details] [diff] [review]
Updated patch

>diff -r 51249e968f24 js/src/js.cpp
>--- a/js/src/js.cpp	Mon Nov 24 11:36:44 2008 +0100
>+++ b/js/src/js.cpp	Mon Nov 24 22:45:05 2008 +0100
>@@ -133,45 +133,101 @@ split_setup(JSContext *cx);
> 
> #ifdef EDITLINE
> JS_BEGIN_EXTERN_C
> extern char     *readline(const char *prompt);
> extern void     add_history(char *line);
> JS_END_EXTERN_C
> #endif
> 
>-static JSBool
>-GetLine(JSContext *cx, char *bufp, FILE *file, const char *prompt) {
>+enum AddLineState {
>+    END_OF_LINE,
>+    END_OF_FILE,
>+    OUT_OF_MEMORY
>+};
>+
>+struct ResizableCharBuffer {
>+    size_t bufferSize;
>+    char   buffer[1];

Nit: call "bufferSize" simply "size" and "buffer" "data". Also, drop "Resizable" from the name of the structure since that property comes from the way the structure is used, not from the way its field are organized.   

>+typedef struct ResizableCharBuffer ResizableCharBuffer;

This is C++ so no need for the typedefs.

>+
>+static AddLineState
>+AddLine(JSContext *cx,
>+        ResizableCharBuffer **pointerToPointer,

SpderMonkey rule for such pointer to in-out arguments is to call it using canonical name for the argument with the sufix p. But since this is C++ you may well use a reference and call it just ResizableCharBuffer *&rbuffer leaving the need to have a local assuming that the compiler will optimize dereferences.

>+        FILE *file,
>+        const char *prompt)
>+{
>+
>+    char *bufp;
>+    ResizableCharBuffer *rbuffer = *pointerToPointer;
>+
> #ifdef EDITLINE
>-    /*
>-     * Use readline only if file is stdin, because there's no way to specify
>-     * another handle.  Are other filehandles interactive?
>-     */
>-    if (file == stdin) {
>-        char *linep = readline(prompt);
>-        if (!linep)
>-            return JS_FALSE;
>-        if (linep[0] != '\0')
>-            add_history(linep);
>-        strcpy(bufp, linep);
>-        JS_free(cx, linep);
>-        bufp += strlen(bufp);
>-        *bufp++ = '\n';
>-        *bufp = '\0';
>-    } else
>-#endif
>-    {
>-        char line[256];
>+    size_t requiredSize;

Nit: a blank line after the local declaration. But since this is C++ you well move
this declaration to the point of use.

>+    char *linep = readline(prompt);
>+    if (!linep)
>+        return END_OF_FILE;
>+    if (linep[0] != '\0')
>+        add_history(linep);
>+    requiredSize = strlen(rbuffer->buffer) + strlen(linep) + 2;

Why "+2" and not "+1"? You need space just for one '\0'.

>+    if ( requiredSize > rbuffer->bufferSize) {

Nit: no french space after ( ever.

>+        size_t newBufferSize = rbuffer->bufferSize * 2;

Micro nit: use just newSize for the local - it is clear from the code that we are talking about the buffer here.

>+        if (newBufferSize < requiredSize)
>+            newBufferSize = requiredSize;
>+        register ResizableCharBuffer *tempBuffer =

Nit: Do not use the word register here - compiler can optimize this on its own. Also, the canonical name for the realloc result would be just tmp.

>+            (ResizableCharBuffer *) realloc(rbuffer,
>+                                            offsetof(ResizableCharBuffer,buffer)
>+                                            + newBufferSize * sizeof(char));

Nit: no need to use sizeof(char) here - by definition it is 1 and the name CharBuffer will remind the reader about that.

>+        if (!tempBuffer) {
>+            return OUT_OF_MEMORY;
>+        } else {

Nit: No need for else after the return. That would also allow to drop braces.

>+            rbuffer = tempBuffer;
>+            rbuffer->bufferSize = newBufferSize;
>+            *pointerToPointer = rbuffer;
>+        }
>+
>+    }
>+    bufp = &(rbuffer->buffer[0]) + strlen(rbuffer->buffer);
>+    strcpy(bufp, linep);
>+    JS_free(cx, linep);

Fix the existing bug: use just "free" here as the readline implementation calls "malloc". That in turn allows to remove the cx local.


>+    bufp += strlen(bufp);
>+    *bufp++ = '\n';
>+    *bufp = '\0';
>+    return END_OF_LINE;
>+
>+#endif
>+    if (*prompt != '\0') {
>         fprintf(gOutFile, prompt);
>         fflush(gOutFile);
>-        if (!fgets(line, sizeof line, file))
>-            return JS_FALSE;
>-        strcpy(bufp, line);
>-    }
>-    return JS_TRUE;
>+    }
>+    bufp = rbuffer->buffer + strlen(rbuffer->buffer);
>+    while(fgets(bufp, rbuffer->bufferSize - strlen(rbuffer->buffer), file)) {
>+        char *newLine = strchr(bufp, '\n');
>+        if (newLine) {
>+            return END_OF_LINE;
>+        }

Nit: no braces around the single line then-part.

>+        if (strlen(rbuffer->buffer) + 1 == rbuffer->bufferSize) {
>+            size_t newBufferSize = 2 * rbuffer->bufferSize;
>+            register ResizableCharBuffer *tempBuffer =
>+                (ResizableCharBuffer *) realloc(rbuffer,
>+                                                offsetof(ResizableCharBuffer,
>+                                                         buffer)
>+                                                +newBufferSize * sizeof(char));
>+            if (!tempBuffer) {
>+                return OUT_OF_MEMORY;
>+            } else {
>+                rbuffer = tempBuffer;
>+                rbuffer->bufferSize = newBufferSize;
>+                *pointerToPointer = rbuffer;
>+            }

Repeat of nits: s/newBufferSize/newSize, drop "register", s/tempBuffer/tmp/, drop * sizeof(char).


>+        }
>+        bufp = rbuffer->buffer + strlen(rbuffer->buffer);
>+    }
>+    return END_OF_FILE;

Non-nit: if the last line of the input does not contain newline, then this will report END_OF_FILE. Then the caller will ignore it as it assumes that END_OF_FILE means no new input. For example, try with the patch without EDITLINE defined:

echo -n 'print(1)' | .../js

Another non-nit: there are way too many strlen(rbuffer->buffer) calls here. Some compilers can optimize them into single call, but in general one cannot assume that. And given that strlen must scan the entire buffer, it is expensive. So a suggestion is to add new length field to the buffer that should contain the current value of the length.
I think the current approach in the patch brings way too match source complexity. So I reiterate that suggestion just to change GetLine so it implements readline semantic of returning the malloced buffer. When the shell starts to support <readline/readline.h> the current efforts to optimize for non-editline platforms is just a waste of efforts.

Also, with GetLine as a wrapper for readline the code in Process can be optimized to use an extra buffer only when more than one line is necessary avoiding that allocation of 4K for the common case of short compilable lines of code with interactive usage.
Is there a reason that we can't go whole-hog C++ and just use std::string here?
(In reply to comment #15)
> Is there a reason that we can't go whole-hog C++ and just use std::string here?

2 reasons: 

1. readline is in C and uses malloc.
2. std::string cannot be used AFAIK without turning exceptions on for oem reporting.
(In reply to comment #16)
> 2 reasons: 
> 
> 1. readline is in C and uses malloc.
> 2. std::string cannot be used AFAIK without turning exceptions on for oem
> reporting.

I mean for oom, out-of-memory, reporting.
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #349828 - Attachment is obsolete: true
Attachment #350965 - Flags: review?(igor)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #350965 - Attachment is obsolete: true
Attachment #351178 - Flags: review?(igor)
Attachment #350965 - Flags: review?(igor)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #351178 - Attachment is obsolete: true
Attachment #351193 - Flags: review?(igor)
Attachment #351178 - Flags: review?(igor)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #351193 - Attachment is obsolete: true
Attachment #351198 - Flags: review?(igor)
Attachment #351193 - Flags: review?(igor)
Attachment #351198 - Flags: review?(igor) → review-
Comment on attachment 351198 [details] [diff] [review]
Updated patch

>diff -r 51249e968f24 js/src/js.cpp
>-static JSBool
>-GetLine(JSContext *cx, char *bufp, FILE *file, const char *prompt) {
>+static char *
>+GetLine(FILE *file, const char * prompt)
>+{
>+    size_t size;
>+    char *buffer;
> #ifdef EDITLINE
>-    /*
>-     * Use readline only if file is stdin, because there's no way to specify
>-     * another handle.  Are other filehandles interactive?
>-     */
>     if (file == stdin) {
>         char *linep = readline(prompt);
>+        errno = 0; //we set it to zero to avoid complaining about inappropriate ioctl for device
>         if (!linep)
>-            return JS_FALSE;
>+            return NULL;

About errno - since the caller is supposed to check errno to distinguish EOF from the error, this code means that that no errors are ever reported. If readline() sets errno to a bogus value on end-of-file, then check for that value and reset errno to zero only for that bogus value.  

>+    size_t strSize = 0;

Nit: call this len.

>+    if (*prompt != '\0') {
>         fprintf(gOutFile, prompt);
>         fflush(gOutFile);
>+    }
>+    size = 80;
>+    buffer = (char *) malloc(size);
>+    if (!buffer)
>+        return NULL;
>+    char *current = buffer;
>+    while (fgets(current, size - strSize, file)) {
>+        strSize = strlen(buffer);

Nit:  this can be optimized is strSize += strlen(current) to avoid scanning the already read
portion of the buffer.


>+        char *t = buffer + strSize - 1;
>+        if (*t == '\n') {
>+            /* Line was read. We remove '\n' and exit. */
>+            *t = '\0';
>+            char *tmp = strdup(buffer);
>+            free(buffer);
>+            return tmp;

Nit: no need to strdup as the code can just call realloc() the buffer itself to shrink its size to the strSize+1. But be aware that on some OS such shrinking realloc may return null. If so, just return the original buffer. 

>+        }
>+        if (strSize + 1 == size) {
>+            size = size * 2;
>+            char *tmp = (char *) realloc(buffer, size);
>+            if (!tmp) {
>+                free(buffer);
>+                return NULL;
>+            }
>+            buffer = tmp;
>+        }
>+        current = buffer + strSize;
>+    }
>+    if (strSize) {
>+        return buffer;
>+    } else {
>+        return NULL;
>+    }

This will leak memory allocated for the buffer for the empty input without any chars. Also the code should check for ferror(file) and return null if so, like in:

if (!ferror(file) && strSize)
    return buffer;
free(buffer);
return NULL; 

Also note that this inconsistent with the buffer shrinking in the loop. So either do not bother to shrink the buffer in the loop as well, or shrink it both there and here.

>@@ -282,33 +306,69 @@ Process(JSContext *cx, JSObject *obj, ch
>     /* It's an interactive filehandle; drop into read-eval-print loop. */
>     lineno = 1;
>     hitEOF = JS_FALSE;
>+    char *tmp;
>+    size_t strSize;
>+    size_t newStrSize;

Nit: decalre tmp and newStrSize in the loop where they are used.
Nit: call "strSize" just "len" and call "newStrSize" "newlen".

>     do {
>-        bufp = buffer;
>-        *bufp = '\0';
> 
>         /*
>          * Accumulate lines until we get a 'compilable unit' - one that either
>          * generates an error (before running out of source) or that compiles
>          * cleanly.  This should be whenever we get a complete statement that
>          * coincides with the end of a line.
>          */
>         startline = lineno;
>         do {
>-            if (!GetLine(cx, bufp, file, startline == lineno ? "js> " : "")) {
>-                hitEOF = JS_TRUE;
>-                break;
>-            }
>-            bufp += strlen(bufp);
>+            errno = 0;
>+            tmp = GetLine(file, startline == lineno ? "js> " : "");

Nit: for clarity call the variable line, not tmp.

>+            if (!tmp) {
>+                if (errno) {
>+                    JS_ReportError(cx, strerror(errno));

The return leaks the buffer. Since free(NULL) is do-nothing operation, this suggests to initialize buffer to null before the loop and check for !buffer to detect the first line.

>+                    return;
>+                } else {

Nit: no else after return.

>+                    hitEOF = JS_TRUE;
>+                    break;
>+                }
>+            }
>+            if (lineno == 1) {
>+                buffer = tmp;
>+                strSize = strlen(buffer);
>+                size = strSize + 1;
>+            } else {
>+                newStrSize = strlen(tmp) + (strSize ? strSize + 1 : 0);

Nit: If the input starts with an empty line, then such line is a compilable unit. So the code can just write newlen = len + 1 + strlen(len).

>+                if (newStrSize + 1 > size) {
>+                    if (newStrSize + 1 > size * 2) {
>+                        size = newStrSize + 1;
>+                    }
>+                    else {
>+                        size *= 2;
>+                    }

Nit: use ? operator here like: size = ... ? newlen + 1 : ssize * 2;

>+                    char *newBuf = (char *) realloc(buffer, size);
>+                    if (!newBuf) {
>+                        free(buffer);
>+                        free(tmp);
>+                        JS_ReportOutOfMemory(cx);
>+                        return;
>+                    }
>+                    buffer = newBuf;
>+                }
>+                char *current = buffer + strSize;
>+                if (startline != lineno)
>+                    *current++ = '\n';
>+                strcpy(current, tmp);

The "if" condition is always true here as this code is executed only from the second line. So there is no need for "current" like in:

    JS_ASSERT(buffer[len] == '\0');
    buffer[len] = '\n';
    strcpy(buffer + len + 1, tmp);
(In reply to comment #22)
> (From update of attachment 351198 [details] [diff] [review])
> 
> >+            if (!tmp) {
> >+                if (errno) {
> >+                    JS_ReportError(cx, strerror(errno));
> 
> The return leaks the buffer. Since free(NULL) is do-nothing operation, this
> suggests to initialize buffer to null before the loop and check for !buffer to
> detect the first line.
I do not know if it is wise to release buffer each time we get compilation unit since we can have a lot of them in the same file. I would prefer to reuse the same buffer to collect all compilation units.

> >+                    hitEOF = JS_TRUE;
> >+                    break;
> >+                }
> >+            }
> >+            if (lineno == 1) {
> >+                buffer = tmp;
> >+                strSize = strlen(buffer);
> >+                size = strSize + 1;
> >+            } else {
> >+                newStrSize = strlen(tmp) + (strSize ? strSize + 1 : 0);
> 
> Nit: If the input starts with an empty line, then such line is a compilable
> unit. So the code can just write newlen = len + 1 + strlen(len).
your suggestion works if we had line before but what if we did not? The code must handle this situation as well. 

> >+                    char *newBuf = (char *) realloc(buffer, size);
> >+                    if (!newBuf) {
> >+                        free(buffer);
> >+                        free(tmp);
> >+                        JS_ReportOutOfMemory(cx);
> >+                        return;
> >+                    }
> >+                    buffer = newBuf;
> >+                }
> >+                char *current = buffer + strSize;
> >+                if (startline != lineno)
> >+                    *current++ = '\n';
> >+                strcpy(current, tmp);
> 
> The "if" condition is always true here as this code is executed only from the
> second line. So there is no need for "current" like in:
> 
>     JS_ASSERT(buffer[len] == '\0');
>     buffer[len] = '\n';
>     strcpy(buffer + len + 1, tmp);

I guess there is misunderstanding here. Because lineno is not the current line for the current compilation unit but it is the current line of the whole file. It does not drop back to 1 until we reach the end of the method. startline is the start line for the current compilation unit so we check the current line against the compilation unit start line and if the current line is not the first line then we need to insert '\n'. No need to insert '\n' if it is the first line of the compilation unit.
Attached patch Updated patch (obsolete) — Splinter Review
Just in the case if Igor accepts my comments to his comments.
Attachment #351198 - Attachment is obsolete: true
Attachment #351397 - Flags: review?(igor)
Attachment #351397 - Flags: review?(igor) → review-
Comment on attachment 351397 [details] [diff] [review]
Updated patch

>diff -r 51249e968f24 js/src/editline/editline.c
>@@ -961,16 +961,18 @@ editinput()
> 	    case CSdispatch:
> 	    case CSstay:
> 		break;
> 	    }
> 	    break;
> 	case CSstay:
> 	    break;
> 	}
>+    if (strlen(Line))
>+        return Line;

This will leak the allocated buffer for Line when strlen(Line) == 0.

>diff -r 51249e968f24 js/src/js.cpp
...
> static void
> Process(JSContext *cx, JSObject *obj, char *filename, JSBool forceTTY)
> {
...
>@@ -282,33 +309,63 @@ Process(JSContext *cx, JSObject *obj, ch

>     /* It's an interactive filehandle; drop into read-eval-print loop. */
>     lineno = 1;
>     hitEOF = JS_FALSE;
>+    size_t len;
>     do {
>-        bufp = buffer;
>-        *bufp = '\0';
> 
>         startline = lineno;
>         do {
>-            if (!GetLine(cx, bufp, file, startline == lineno ? "js> " : "")) {
>+            errno = 0;
>+            char *line = GetLine(file, startline == lineno ? "js> " : "");
>+            if (!line) {
>+                if (errno) {
>+                    JS_ReportError(cx, strerror(errno));
>+                    if (buffer)
>+                        free(buffer);

Here buffer is not initialized on the first iteration and free may crash. So set buffer to null before the loop. That would also allow to drop if (buffer) check since it is perfectly OK to call free(NULL). 

>+                    return;
>+                }
>                 hitEOF = JS_TRUE;
>                 break;
>             }
>-            bufp += strlen(bufp);
>+            if (lineno == 1) {

With buffer set to NULL before the loop it would be better to check for !buffer here, not lineno, to simplify the job of following the code for a human.
What's the status here?
I am planning to update the patch tomorrow
Attachment #351397 - Attachment is obsolete: true
Attachment #354942 - Flags: review?(igor)
Comment on attachment 354942 [details] [diff] [review]
Patch updated according to Igor's comments

> static void
> Process(JSContext *cx, JSObject *obj, char *filename, JSBool forceTTY)
> {
>+            } else {
>+                size_t newlen = strlen(line) + (len ? len + 1 : 0);

Here len is zero only if the first lines are empty. But this is not possible since for the empty buffer JS_BufferIsCompilableUnit returns true and the loop terminates. Thus the condition len != 0 is always true here. Also, right it like:

size_t newlen = len + 1 + strlen(line)

and *comment* that 1 comes from appending '\n' between lines. 

...
>+                char *current = buffer + len;
>+                if (startline != lineno)
>+                    *current++ = '\n';

Here startline != lineno is always since the first line is simply assigned to buffer. So the condition can be dropped.

>+                strcpy(current, line);
>+                len = newlen;
>+                free(line);
>+            }
>             lineno++;
>         } while (!JS_BufferIsCompilableUnit(cx, obj, buffer, strlen(buffer)));

Use here len, not strlen(buffer), to avoid useless scanning of the buffer one more time in strlen in search for '\0'.
Attachment #354942 - Flags: review?(igor)
Attachment #354942 - Attachment is obsolete: true
Attachment #356725 - Flags: review?(igor)
Attachment #356725 - Flags: review?(igor) → review+
Landed to tracemonkey http://hg.mozilla.org/tracemonkey/rev/56128cae91c9
Whiteboard: DUPEME [sg:high] → fixed-in-tracemonkey [sg:high]
http://hg.mozilla.org/mozilla-central/rev/56128cae91c9
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
did this make it to the 1.9.1 branch?
Flags: blocking1.9.1?
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d734cc698a82
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Whiteboard: fixed-in-tracemonkey [sg:high] → fixed-in-tracemonkey [sg:nse] local critical bug only if js shell present
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13? → blocking1.9.0.13+
Attachment #356725 - Flags: approval1.9.0.14+
Comment on attachment 356725 [details] [diff] [review]
Updated and refreshed patch

Approved for 1.9.0.14, a=dveditz for release-drivers
Talked with Blake. We're not going to take this for 1.9.0.14 and will push it to 1.9.0.15. He's going to work on a roll-up patch.
Flags: blocking1.9.0.14+ → blocking1.9.0.15+
Attachment #356725 - Flags: approval1.9.0.14+
Assigning this to Blake since he's going to be working on the 1.9.0 patch... Blake, you didn't forget right?
Assignee: andrei → mrbkap
Attached patch For 1.9.0Splinter Review
I think the easiest way to review this is to diff against trunk and make sure all of the relevant code has changed. I'll attach a full diff in a second.
Attachment #400627 - Flags: review?(igor)
Attachment #400627 - Flags: review?(igor) → review+
Attachment #400627 - Flags: approval1.9.0.15?
Depends on: 475449
Comment on attachment 400627 [details] [diff] [review]
For 1.9.0

Approved for 1.9.0.15, a=dveditz
Attachment #400627 - Flags: approval1.9.0.15? → approval1.9.0.15+
Checking in js/src/editline/editline.c;
/cvsroot/mozilla/js/src/editline/editline.c,v  <--  editline.c
new revision: 1.9; previous revision: 1.8
done
Checking in js/src/js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.215; previous revision: 3.214
done
Keywords: fixed1.9.0.15
Bob, can you verify that this is fixed in 1.9.0.15 with a nightly?
(In reply to comment #41)
> Checking in js/src/editline/editline.c;
> /cvsroot/mozilla/js/src/editline/editline.c,v  <--  editline.c
> new revision: 1.9; previous revision: 1.8
> done
> Checking in js/src/js.c;
> /cvsroot/mozilla/js/src/js.c,v  <--  js.c
> new revision: 3.215; previous revision: 3.214
> done

this broke building the shell on winxp.
Attached file compile errors
Checking in js/src/js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.216; previous revision: 3.215
done

fixes comment 44.
mrbkap, thanks!

Wes, do you have a test case I can use? I tried a variable assignment with a really really long string and one with a literal array with lots and lots of elements, but neither crashed on Windows prior to the patch.
Attached file Trivial test case
Sure, Bob;

The test case I'm attachingcrashes here (Solaris 10, sparc) due to the buffer overrun described in comment 1. 

Note that you must invoke the js shell with the -i switch to get readline involved, and you can't word-wrap the ,,,,, line.

If it doesn't crash for you, you might be using a different readline library (does js.c still support two?) or you might just be getting lucky and not overrunning anything important.

I have confirmed "fixed" in a recent tracemonkey build, and "crashes" in an old Spidermonkey CVS build from Dec 17 2007.
Ok. I initially didn't use the -i. I can reproduce this on Mac but not Windows 1.9.0 shell built prior to the patch using editline. Is that sufficient to test or do I need to use readline? verifying no crash on mac 1.9.0, 1.9.1, 1.9.2, 1.9.3 and with a much larger input a script stack space quota is exhausted exception.
Status: RESOLVED → VERIFIED
OS: Windows XP → All
Hardware: x86 → All
This patch has rendered interactive mode completely unusable: only the first statement will ever execute.

$ Darwin_DBG.OBJ/js
js> 1
1
js> 1
typein:2: SyntaxError: illegal character:
typein:2: 
typein:2: ^

Replicated on Darwin, WinXP (thanks bc), Solaris 10. Verified that this bug is root cause by reverting js.c to 3.214.

Suggest we revert this until it can be fixed properly.
Resolution: FIXED → INCOMPLETE
Depends on: 529535
Resolution: INCOMPLETE → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.