349527 - GC hazard when copying JSErrorReport

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

exn_newPrivate from js/src/jsexn.c contains the following code:

    /*
     * We don't need to copy linebuf and tokenptr, because they
     * point into the deflated string cache.  (currently?)
     */
    newReport->linebuf = report->linebuf;  
    newReport->tokenptr = report->tokenptr;

This is wrong since, for example, any finally block can trigger GC before the error is reported. GC then releases the cache leading later to accessing of freed memory in the error handler.

For example, consider the following example for JS shell:

var invalid_script = Array(1<<12).join('\n')+"bad!bad!bad";

try {
        Function(invalid_script);
} finally {  
        try {
                Function(invalid_script);
        } catch (e) {
        }
        gc();
}

When the line gc(); is commented out, the shell prints:

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/s/x.js
/home/igor/s/x.js:4099: SyntaxError: missing ; before statement:
/home/igor/s/x.js:4099: bad!bad!bad
/home/igor/s/x.js:4099: ...^

With gc() call present the result is:

~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/s/x.js
before 9232, after 9232, break 0819c000
/home/igor/s/x.js:4099: SyntaxError: missing ; before statement:
/home/igor/s/x.js:4099:
/home/igor/s/x.js:4099: ...^

I.e. the shell is accessing freed memory when it prints the error report.

Comment 1

[Igor Bukanov]

Attachment: Fix

The patch makes a copy of all the necessary strings using the single buffer to avoid numerous malloc/free calls.

Comment 2

[Igor Bukanov]

Attachment: Fix for real

The previous attachment got the wrong patch.

Comment 3

[Brendan Eich [:brendan]]

mrbkap is on the road, we may need shaver's help for second review, but second review is not mandatory.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 234783 [details] [diff] [review]
Fix for real

>+#define COPY_JS_CHARS(dest, src)                                              \
>+    JS_BEGIN_MACRO                                                            \
>+        size_t size_ = (js_strlen(src) + 1) * sizeof(jschar);                 \
>+        if (!newReport) {                                                     \
>+            overflow |= (sizeof(jschar) > (size_t)-1 - maxSize);              \

In light of JS_ROUNDUP (next line, cited below), this should be (sizeof(jschar) - 1 > ...).  Which the compiler should turn into ... == 0, of course.

>+            maxSize = JS_ROUNDUP(maxSize, sizeof(jschar));                    \
>+            overflow |= (size_ > (size_t)-1 - maxSize);                       \
>+            maxSize += size_;                                                 \
>+        } else {                                                              \
>+            *dest = (jschar *)(JS_ROUNDUP((jsuword)cursor, sizeof(jschar)));  \
>+            memcpy(cursor, src, size_);                                       \
>+            cursor += size_;                                                  \
>+        }                                                                     \
>+    JS_END_MACRO
>+
>+#define ALLOC_MEM(array, nelems, elemtype)                                    \
>+    JS_BEGIN_MACRO                                                            \
>+        size_t size_ = nelems * sizeof(elemtype);                             \
>+        if (!newReport) {                                                     \
>+            overflow |= (sizeof(elemtype) > (size_t)-1 - maxSize);            \

Same comment: sizeof(elemtype) - 1

>+    for (;;) {
>+        COPY_C_CHARS(&newReport->filename, report->filename);
>+        if (report->linebuf)
>+            COPY_C_CHARS(&newReport->linebuf, report->linebuf);
>+        if (report->tokenptr)
>+            COPY_C_CHARS(&newReport->tokenptr, report->tokenptr);
>         if (report->uclinebuf)
>-            JS_free(cx, (void *)report->uclinebuf);

Looping is cool, since macros help keep sizing and copying/allocating code together, but I probably would have just open-coded it, for slight code size savings over loop-containing-ifs.  If you can, or I can make time, prove that there is no size savings to open-coding, and I'll be completely in favor.

>+        /* End of calculation phase. */
>+        overflow |= (sizeof(jschar) > (size_t)-1 - sizeof(jsuword));

This can't be right.

Thanks for taking this on -- it's looking a lot better.

/be

Comment 5

[Igor Bukanov]

Attachment: Fix v2

My previous patch was broken besides being to generic and over paranoid: there is no overflow there and no alignment padding is necessary with a proper layout. So this version just explicitly calculates sizes and then do copy in separated parts.

For easier reviewing here is the new version of CopyErrorReport:

static JSErrorReport *
CopyErrorReport(JSContext *cx, JSErrorReport *report)
{
    /*
     * We use a single malloc block to make a deep copy of JSErrorReport with
     * the following layout:
     *   JSErrorReport
     *   array of copies of report->messageArgs
     *   jschar array with all characters from report->messageArgs
     *   jschar array with characters for ucmessage
     *   jschar array with characters uclinebuf and uctokenptr
     *   char array with characters for linebuf and tokenptr
     *   char array with characters for filename
     * Such layout together with the properties enforced by the following
     * asserts does not need any extra alignment padding.
     */
    JS_STATIC_ASSERT(sizeof(JSErrorReport) % sizeof(const char *) == 0);
    JS_STATIC_ASSERT(sizeof(const char *) % sizeof(jschar) == 0);

    size_t filenameSize;
    size_t linebufSize;
    size_t uclinebufSize;
    size_t ucmessageSize;
    size_t i, argsArraySize, argsCopySize, argSize;
    size_t mallocSize;
    JSErrorReport *copy;
    uint8 *cursor;

#define JS_CHARS_SIZE(jschars) ((js_strlen(jschars) + 1) * sizeof(jschar))

    filenameSize = strlen(report->filename) + 1;
    linebufSize = (report->linebuf) ? strlen(report->linebuf) + 1 : 0;
    uclinebufSize = (report->uclinebuf) ? JS_CHARS_SIZE(report->uclinebuf) : 0;
    ucmessageSize = 0;
    argsArraySize = 0;
    argsCopySize = 0;
    if (report->ucmessage) {
        ucmessageSize = JS_CHARS_SIZE(report->ucmessage);
        if (report->messageArgs) {
            for (i = 0; report->messageArgs[i]; ++i)
                argsCopySize += JS_CHARS_SIZE(report->messageArgs[i]);

            /* Non-null messageArgs should have at least one non-null arg. */
            JS_ASSERT(i != 0);
            argsArraySize = (i + 1) * sizeof(const jschar *);
        }
    }

    /*
     * The mallocSize can not overflow since it represents the sum of the
     * sizes of already allocated objects.
     */
    mallocSize = sizeof(JSErrorReport) + argsArraySize + argsCopySize +
                 ucmessageSize + uclinebufSize + linebufSize + filenameSize;
    cursor = (uint8 *)JS_malloc(cx, mallocSize);
    if (!cursor)
        return NULL;

    copy = (JSErrorReport *)cursor;
    memset(cursor, 0, sizeof(JSErrorReport));
    cursor += sizeof(JSErrorReport);

    if (argsArraySize != 0) {
        copy->messageArgs = (const jschar **)cursor;
        cursor += argsArraySize;
        for (i = 0; report->messageArgs[i]; ++i) {
            copy->messageArgs[i] = (const jschar *)cursor;
            argSize = JS_CHARS_SIZE(report->messageArgs[i]);
            memcpy(cursor, report->messageArgs[i], argSize);
            cursor += argSize;
        }
        copy->messageArgs[i] = NULL;
        JS_ASSERT(argsCopySize == cursor - (uint8 *)copy->messageArgs[0]);
    }

    if (report->ucmessage) {
        copy->ucmessage = (const jschar *)cursor;
        memcpy(cursor, report->ucmessage, ucmessageSize);
        cursor += ucmessageSize;
    }

    if (report->uclinebuf) {
        copy->uclinebuf = (const jschar *)cursor;
        memcpy(cursor, report->uclinebuf, uclinebufSize);
        cursor += uclinebufSize;
        if (report->uctokenptr) {
            copy->uctokenptr = copy->uclinebuf + (report->uctokenptr -
                                                  report->uclinebuf);
        }
    }

    if (report->linebuf) {
        copy->linebuf = (char *)cursor;
        memcpy(cursor, report->linebuf, linebufSize);
        cursor += linebufSize;
        if (report->tokenptr) {
            copy->tokenptr = copy->linebuf + (report->tokenptr -
                                              report->linebuf);
        }
    }

    copy->filename = (const char *)cursor;
    memcpy(cursor, report->filename, filenameSize);
    JS_ASSERT(cursor + filenameSize == (uint8 *)copy + mallocSize);

    /* Copy non-pointer members. */
    copy->lineno = report->lineno;
    copy->errorNumber = report->errorNumber;

    /* Note that this is before it gets flagged with JSREPORT_EXCEPTION */
    copy->flags = report->flags;

#undef JS_CHARS_SIZE
    return copy;
}

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 235392 [details] [diff] [review]
Fix v2

That's the ticket -- non-increasing size order sorts into aligned segments, for power-of-two-aligned segment sizes.

>+    linebufSize = (report->linebuf) ? strlen(report->linebuf) + 1 : 0;
>+    uclinebufSize = (report->uclinebuf) ? JS_CHARS_SIZE(report->uclinebuf) : 0;

Nit: usual style doesn't parenthesize ?: condition unless it has lower-predence operators with spaces around them -- more of a visual thing to help see the condition than anything else.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 235392 [details] [diff] [review] [edit])
> That's the ticket -- non-increasing size order

I mean alignment grain size order, of course.

/be

Comment 8

[Igor Bukanov]

Attachment: Fix v2b

Patch to commit with nits addresses and the following extra change to emphasis the type of JSErrorReport linebuf in the same way as the patch emphasis the constness of the rest of JSErrorReporter strings. 

-        copy->linebuf = (char *)cursor;
+        copy->linebuf = (const char *)cursor;

Comment 9

[Igor Bukanov]

I committed the patch from comment 8 to the trunk:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.72; previous revision: 3.71

Comment 10

[Bob Clary [:bc] (inactive)]

I don't know how to modify the testcase so the error framework can detect success or failure. 

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> I don't know how to modify the testcase so the error framework can detect
> success or failure. 
> 

I will try to modify the test case to crash. It must be a way to cause segfault while reading freed memory.

Comment 12

[Mike Schroepfer]

Comment on attachment 235542 [details] [diff] [review]
Fix v2b

 a=schrep/beltnzer for drivers.

Comment 13

[Brendan Eich [:brendan]]

Need a fix for bug 350393 before landing a consolidated 1.8 branch patch.

The patch that did land introduced a gcc warning:

jsexn.c:164: warning: comparison between signed and unsigned

Igor, can you fix that and include it in the branch patch?  Thanks.

/be

Comment 14

[Igor Bukanov]

Attachment: Fix for assert warning

There is a value in debugger builds besides getting better stack traces.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 235701 [details] [diff] [review]
Fix for assert warning

Nice, + to avoid a cast.

/be

Comment 16

[Igor Bukanov]

The committed patch introduced a leak as it does not release in  exn_finalize JSExnPrivate.

Comment 17

[Igor Bukanov]

Attachment: Fix for the leak

Note that this has nothing to do with bug 350393. That is elusive. Perhaps different malloc size expose latent problem?

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 235717 [details] [diff] [review]
Fix for the leak

r=me.

/be

Comment 19

[Brendan Eich [:brendan]]

1.8 branch patch wanted.

/be

Comment 20

[Igor Bukanov]

Attachment: Fix for the leak and filename null check

I hope this is the final damage fix.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 235867 [details] [diff] [review]
Fix for the leak and filename null check

r=me.  Guess we don't need the other bug except for regression testing.

/be

Comment 22

[Igor Bukanov]

I committed the patch from comment 20 to the trunk:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.74; previous revision: 3.73

Comment 23

[Igor Bukanov]

Attachment: Full patch for 1.8.1 branch

This is a union of 3 committed patches.

Comment 24

[Mike Schroepfer]

Comment on attachment 235918 [details] [diff] [review]
Full patch for 1.8.1 branch

a=schrep for drivers.

Comment 25

[Igor Bukanov]

I committed the patch from comment 23 to MOZILLA_1_8_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.8; previous revision: 3.50.4.7
done

Comment 26

[Daniel Veditz [:dveditz]]

Brendan: do we need this in 1.8.0.x? I assume we'll need a new patch if so since 1.8.1 is now js1.7

Comment 27

[Brendan Eich [:brendan]]

This is an old bug, so it could be fixed in other branches.  Igor's patch should back-port pretty cleanly, I think.  Igor?

/be

Comment 28

[Igor Bukanov]

Comment on attachment 235918 [details] [diff] [review]
Full patch for 1.8.1 branch

The patch applies to 1.8.0 as is.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 235918 [details] [diff] [review]
Full patch for 1.8.1 branch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 30

[Igor Bukanov]

Attachment: Full patch for 1.8.0 branch

I was wrong when wrote that the patch applies to 1.8.0 as is. 

It uses JS_STATIC_ASSERT macro from js/src/jsutil.h that exists only on 1.8.1 and later. Since the macro uses some C-language trickery that once proved to be problematic with older MSVC in a particular usage, I prefer not to add the macro  to 1.8.0 branch. So here is a patch without compilation-time asserts.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 240216 [details] [diff] [review]
Full patch for 1.8.0 branch

approval1.8.0.8+ moved to the correct patch, a=dveditz

Comment 32

[Igor Bukanov]

I committed the patch from comment 30 to MOZILLA_1_8_0_BRANCH:

Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.4.2.3; previous revision: 3.50.4.4.2.2
done