620241 - Decompile leaks argv in JSOP_NEW.. when LOCAL_ASSERT fails

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[timeless]

1297 #define LOCAL_ASSERT_CUSTOM(expr, BAD_EXIT)                                   \
1298     JS_BEGIN_MACRO                                                            \
1299         JS_ASSERT(expr);                                                      \
1300         if (!(expr)) { BAD_EXIT; }                                            \
1301     JS_END_MACRO
1302 
1303 #define LOCAL_ASSERT_RV(expr, rv)                                             \
1304     LOCAL_ASSERT_CUSTOM(expr, return (rv))

1825 Decompile(SprintStack *ss, jsbytecode *pc, intN nb, JSOp nextop)

1869 #define LOCAL_ASSERT(expr)    LOCAL_ASSERT_RV(expr, NULL)

3584               case JSOP_NEW:
3585               case JSOP_CALL:
3586               case JSOP_EVAL:
3587               case JSOP_FUNCALL:
3588               case JSOP_FUNAPPLY:
3589                 argc = GET_ARGC(pc);
3590                 argv = (char **)
3591                     cx->malloc((size_t)(argc + 1) * sizeof *argv);
3592                 if (!argv)
3593                     return NULL;
3594 
3595                 op = JSOP_SETNAME;
3596                 ok = JS_TRUE;
3597                 for (i = argc; i > 0; i--)
3598                     argv[i] = JS_strdup(cx, POP_STR());
3599 
3600                 /* Skip the JSOP_PUSHOBJ-created empty string. */
3601                 LOCAL_ASSERT(ss->top >= 2);

Comment 1

[timeless]

Attachment: patch

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 498618 [details] [diff] [review]
patch

Review of attachment 498618 [details] [diff] [review]:

To me, the leak looks worse than that; all the elements of argv are leaked as well. The cleanup code is on line 3674:

>                for (i = 0; i <= argc; i++)
>                    cx->free_(argv[i]);
>                cx->free_(argv);

Needs a `goto cleanup` sort of fix.

Comment 3

[Dan Gohman [:sunfish]]

The relevant code appears to have been substantially rewritten. Can this be closed?

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.