Note: There are a few cases of duplicates in user autocompletion which are being worked on.

GC hazard when copying JSErrorReport

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({fixed1.8.0.8, fixed1.8.1})

Trunk
fixed1.8.0.8, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.8 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][need testcase])

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 234782 [details] [diff] [review]
Fix

The patch makes a copy of all the necessary strings using the single buffer to avoid numerous malloc/free calls.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #234782 - Flags: superreview?(mrbkap)
Attachment #234782 - Flags: review?(brendan)
(Assignee)

Comment 2

11 years ago
Created attachment 234783 [details] [diff] [review]
Fix for real

The previous attachment got the wrong patch.
Attachment #234782 - Attachment is obsolete: true
Attachment #234783 - Flags: superreview?(mrbkap)
Attachment #234783 - Flags: review?(brendan)
Attachment #234782 - Flags: superreview?(mrbkap)
Attachment #234782 - Flags: review?(brendan)
Whiteboard: [sg:critical?]
mrbkap is on the road, we may need shaver's help for second review, but second review is not mandatory.

/be
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
(Assignee)

Comment 5

11 years ago
Created attachment 235392 [details] [diff] [review]
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;
}
Attachment #234783 - Attachment is obsolete: true
Attachment #235392 - Flags: review?(brendan)
Attachment #234783 - Flags: superreview?(mrbkap)
Attachment #234783 - Flags: review?(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
Attachment #235392 - Flags: review?(brendan) → review+
(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
(Assignee)

Comment 8

11 years ago
Created attachment 235542 [details] [diff] [review]
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;
Attachment #235392 - Attachment is obsolete: true
(Assignee)

Comment 9

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #235542 - Flags: approval1.8.1?

Comment 10

11 years ago
I don't know how to modify the testcase so the error framework can detect success or failure. 
Flags: in-testsuite-
(Assignee)

Comment 11

11 years ago
(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.

Updated

11 years ago
Whiteboard: [sg:critical?] → [baking until 8/28][sg:critical?]
Depends on: 350393

Comment 12

11 years ago
Comment on attachment 235542 [details] [diff] [review]
Fix v2b

 a=schrep/beltnzer for drivers.
Attachment #235542 - Flags: approval1.8.1? → approval1.8.1+
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
Flags: blocking1.8.1?
(Assignee)

Comment 14

11 years ago
Created attachment 235701 [details] [diff] [review]
Fix for assert warning

There is a value in debugger builds besides getting better stack traces.
Attachment #235542 - Attachment is obsolete: true
Attachment #235701 - Flags: review?(brendan)
Comment on attachment 235701 [details] [diff] [review]
Fix for assert warning

Nice, + to avoid a cast.

/be
Attachment #235701 - Flags: review?(brendan) → review+
(Assignee)

Comment 16

11 years ago
The committed patch introduced a leak as it does not release in  exn_finalize JSExnPrivate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

11 years ago
Created attachment 235717 [details] [diff] [review]
Fix for the leak

Note that this has nothing to do with bug 350393. That is elusive. Perhaps different malloc size expose latent problem?
Attachment #235717 - Flags: review?(brendan)

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 235717 [details] [diff] [review]
Fix for the leak

r=me.

/be
Attachment #235717 - Flags: review?(brendan) → review+
1.8 branch patch wanted.

/be
(Assignee)

Comment 20

11 years ago
Created attachment 235867 [details] [diff] [review]
Fix for the leak and filename null check

I hope this is the final damage fix.
Attachment #235717 - Attachment is obsolete: true
Attachment #235867 - Flags: review?(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
Attachment #235867 - Flags: review?(brendan) → review+
(Assignee)

Comment 22

11 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

11 years ago
Created attachment 235918 [details] [diff] [review]
Full patch for 1.8.1 branch

This is a union of 3 committed patches.
Attachment #235918 - Flags: approval1.8.1?

Comment 24

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

a=schrep for drivers.
Attachment #235918 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 25

11 years ago
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
Keywords: fixed1.8.1
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.8+
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
Flags: blocking1.8.0.8+ → blocking1.8.0.8?
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
(Assignee)

Comment 28

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

The patch applies to 1.8.0 as is.
Attachment #235918 - Flags: approval1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.8+
Comment on attachment 235918 [details] [diff] [review]
Full patch for 1.8.1 branch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #235918 - Flags: approval1.8.0.8? → approval1.8.0.8+
(Assignee)

Comment 30

11 years ago
Created attachment 240216 [details] [diff] [review]
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.
Attachment #240216 - Flags: approval1.8.0.8?
Attachment #235918 - Flags: approval1.8.0.8+ → approval1.8.0.8-
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
Attachment #240216 - Flags: approval1.8.0.8? → approval1.8.0.8+
(Assignee)

Comment 32

11 years ago
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
Keywords: fixed1.8.0.8
Whiteboard: [baking until 8/28][sg:critical?] → [sg:critical?]

Updated

11 years ago
Whiteboard: [sg:critical?] → [sg:critical?][need testcase]
Group: security
You need to log in before you can comment on or make changes to this bug.