Conditional jump or move depends on uninitialised value(s)

[saghul]

Caught using valgrind:

valgrind --leak-check=full --show-leak-kinds=all ./build/run-test262 -m -c test262.conf -a
==44768== Memcheck, a memory error detector
==44768== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==44768== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info
==44768== Command: ./build/run-test262 -m -c test262.conf -a
==44768==
==44768== Conditional jump or move depends on uninitialised value(s)
==44768==    at 0x128ABC: find_line_num.isra.0 (quickjs.c:6387)
==44768==    by 0x139799: build_backtrace (quickjs.c:6510)
==44768==    by 0x13A053: JS_ThrowError2 (quickjs.c:6616)
==44768==    by 0x13A66B: JS_ThrowTypeError (quickjs.c:6653)
==44768==    by 0x13C073: __JS_ThrowTypeErrorAtom (quickjs.c:6677)
==44768==    by 0x13C073: JS_ThrowTypeErrorInvalidClass (quickjs.c:6799)
==44768==    by 0x13C073: JS_GetOpaque2 (quickjs.c:9844)
==44768==    by 0x13C073: js_array_buffer_get_byteLength (quickjs.c:48668)
==44768==    by 0x16D75F: js_call_c_function (quickjs.c:14380)
==44768==    by 0x12C22D: JS_CallInternal (quickjs.c:14561)
==44768==    by 0x135E5F: JS_CallFree (quickjs.c:17008)
==44768==    by 0x12DA2E: JS_CallInternal (quickjs.c:15830)
==44768==    by 0x12BDAC: JS_CallInternal (quickjs.c:14933)
==44768==    by 0x12C80E: JS_CallInternal (quickjs.c:14969)
==44768==    by 0x135E5F: JS_CallFree (quickjs.c:17008)
==44768==
==44768== Conditional jump or move depends on uninitialised value(s)
==44768==    at 0x128ABC: find_line_num.isra.0 (quickjs.c:6387)
==44768==    by 0x139799: build_backtrace (quickjs.c:6510)
==44768==    by 0x131DBD: JS_CallInternal (quickjs.c:16953)
==44768==    by 0x12BDAC: JS_CallInternal (quickjs.c:14933)
==44768==    by 0x135E5F: JS_CallFree (quickjs.c:17008)
==44768==    by 0x12DA2E: JS_CallInternal (quickjs.c:15830)
==44768==    by 0x12BDAC: JS_CallInternal (quickjs.c:14933)
==44768==    by 0x12C80E: JS_CallInternal (quickjs.c:14969)
==44768==    by 0x135E5F: JS_CallFree (quickjs.c:17008)
==44768==    by 0x19F244: JS_EvalFunctionInternal (quickjs.c:32039)
==44768==    by 0x19F514: __JS_EvalInternal (quickjs.c:32195)
==44768==    by 0x1A0096: JS_EvalInternal (quickjs.c:32213)
==44768==    by 0x1A0096: JS_EvalThis (quickjs.c:32244)
==44768==    by 0x1A0096: JS_Eval (quickjs.c:32252)
==44768==

By the look of it, the value of pc_value passed to find_line_num is the one not initialized.

[saghul]

Now we have the source:

.....................................-..==2683== Conditional jump or move depends on uninitialised value(s)
==2683==    at 0x12939E: find_line_num.constprop.0 (quickjs.c:6387)
==2683==    by 0x13D8B8: build_backtrace (quickjs.c:6510)
==2683==    by 0x13E2C3: JS_ThrowError2 (quickjs.c:6616)
==2683==    by 0x13E902: JS_ThrowTypeError (quickjs.c:6653)
==2683==    by 0x140279: __JS_ThrowTypeErrorAtom (quickjs.c:6677)
==2683==    by 0x140279: JS_ThrowTypeErrorInvalidClass (quickjs.c:6799)
==2683==    by 0x140279: JS_GetOpaque2 (quickjs.c:9844)
==2683==    by 0x140279: js_array_buffer_get_byteLength (quickjs.c:48668)
==2683==    by 0x1733E4: js_call_c_function (quickjs.c:14380)
==2683==    by 0x12D6D2: JS_CallInternal (quickjs.c:14561)
==2683==    by 0x13A0AF: JS_CallFree (quickjs.c:17008)
==2683==    by 0x12FBAD: JS_CallInternal (quickjs.c:15830)
==2683==    by 0x12D26D: JS_CallInternal (quickjs.c:14933)
==2683==    by 0x12E187: JS_CallInternal (quickjs.c:14969)
==2683==    by 0x13A0AF: JS_CallFree (quickjs.c:17008)
==2683==  Uninitialised value was created by a stack allocation
==2683==    at 0x12CE70: JS_CallInternal (quickjs.c:14487)

[bnoordhuis]

Do you remember at what commit that is?

[saghul]

Current tip of tree, you can check here: https://github.com/quickjs-ng/quickjs/actions/runs/8364487250

[saghul]

Looks like the PC not always set on the JS stack frame structure that we allocate on the stack. I was tempted to send a patch zeroing out the structure but feels kinda wrong?

[bnoordhuis]

Do you mean sf->cur_pc? Does zeroing it out fix the warning?

[saghul]

Yeah that's the one. I haven't tested it yet, hope to do that tonight! 🥃

[chqrlie]

zeroeing the stack frame may hide the problem and will add overhead to every function call.
It would be preferable to investigate by filling the structure with invalid data such as 0xCC to make the problem occur more systematically and figure what condition causes an exception to be thrown without a valid pointer in cur_pc.

[saghul]

I have found the smallest repro case!

"use strict";

ArrayBuffer.prototype.byteLength;

[saghul]

This is the smallest change I could come up with, which fixes it: #328

I couldn't quite follow what path lead to it not being set :-( However, since we initialize the stack frame there, I guess it makes sense to set cur_pc to a sensible value...

[chqrlie]

I couldn't quite follow what path lead to it not being set :-( However, since we initialize the stack frame there, I guess it makes sense to set cur_pc to a sensible value...

The scenario is this:

• the stack frame has an invalid cur_pc until explicitly set by an opcode handler.
• the byte code evaluated is OP_get_field to get the byteLength property.
• this property is a getter, so the c function is called (an interpreter getter would have the same problem).
• this getter throws an exception for an invalid class error.
• the Error constructor walk the stack frames and uses the invalid cur_pc to determine the line number and crashes.

It is quite surprising that this problem has not surfaced before.
To fix this problem, sf-.cur_pc should always be a valid pointer or a null pointer for all active stack frames.

Also note that the stack dump construction is supposed to be delayed until it is needed for display or debugging and this bug shows that it is not the case since build_backtrace construct the complete stack trace in the JS_ThrowError2 call.

The stack trace cannot be built only when accessed because the stack frames would have been discard by that time, yet the call stack could be stored incrementally in the alternate return path along the call chain (in the exception: code for JS_CallInternal).

This would produce a more precise stack trace with the actual cur_pc values, without the need to set sf->cur_pc = pc; explicitly as is done in the OP_call_method handler.

On the other hand, to prepare for a much needed debugger that would need an exact stack trace at time the error constructor is called, this incremental approach is inappropriate.

Conclusion:

• sf->cur_pc must be valid or null for all active stack frames.
• All opcodes that may throw an exception must set sf->cur_pc = pc before the potential error.
• This must be added to OP_get_field, OP_get_field_ic and many other opcode handlers whenever an Error may be thrown. This should be done carefully to try and minimize the overhead.

[saghul]

Thanks for the explainer! I shall try to judiciously place those.

[saghul]

To fix this problem, sf-.cur_pc should always be a valid pointer or a null pointer for all active stack frames.

Not sure I fully get this. When set it means it's not an active stack frame?

Also note that the stack dump construction is supposed to be delayed until it is needed for display or debugging and this bug shows that it is not the case since build_backtrace construct the complete stack trace in the JS_ThrowError2 call.

In JS_ThrowError I can see:

    /* the backtrace is added later if called from a bytecode function */
    sf = rt->current_stack_frame;
    add_backtrace = !rt->in_out_of_memory &&
        (!sf || (JS_GetFunctionBytecode(sf->cur_func) == NULL));
    return JS_ThrowError2(ctx, error_num, fmt, ap, add_backtrace);

But when I check build_backltrace I can see the function does seem to have bytecode? (that's the path that leads to the error). I suppose this is the stack frame related to the code in my JS file and not the actual array bytelength function.

The stack trace cannot be built only when accessed because the stack frames would have been discard by that time, yet the call stack could be stored incrementally in the alternate return path along the call chain (in the exception: code for JS_CallInternal).

Not sure I understand what you mean here 😅 I do see cur_pc being set to pc in the exception label, but it looks like it might be too late, in case an exception which needs to generate a backtrace happens beforehand...

I'm updating the PR with the cases I found thanks for Valgrind, and I'm also making it possible to run Valgrind against a specific branch so we can know some more.

I am a bit out of my depth here, so I appreciate the guidance @chqrlie !

[chqrlie]

To fix this problem, sf->cur_pc should always be a valid pointer or a null pointer for all active stack frames.

Not sure I fully get this. When set it means it's not an active stack frame?

An active stack frame is one that is in a call chain. sf->cur_pc is null for functions that are not bytecoded.

Also note that the stack dump construction is supposed to be delayed until it is needed for display or debugging and this bug shows that it is not the case since build_backtrace construct the complete stack trace in the JS_ThrowError2 call.

In JS_ThrowError I can see:

    /* the backtrace is added later if called from a bytecode function */
    sf = rt->current_stack_frame;
    add_backtrace = !rt->in_out_of_memory &&
        (!sf || (JS_GetFunctionBytecode(sf->cur_func) == NULL));
    return JS_ThrowError2(ctx, error_num, fmt, ap, add_backtrace);

Yes, this delays the construction for most errors triggered from the bytecode to after the goto exception, yet if the call from JS_CallInternal indirectly calls a byte-coded function, the calling stack frame will not have the correct cur_pc and this will cause a problem when this recursive call does its own goto exception for the error it triggers.

But when I check build_backtrace I can see the function does seem to have bytecode? (that's the path that leads to the error). I suppose this is the stack frame related to the code in my JS file and not the actual array bytelength function.

The stack trace cannot be built only when accessed because the stack frames would have been discarded by that time, yet the call stack could be stored incrementally in the alternate return path along the call chain (in the exception: code for JS_CallInternal).

Not sure I understand what you mean here 😅 I do see cur_pc being set to pc in the exception label, but it looks like it might be too late, in case an exception which needs to generate a backtrace happens beforehand...

Yes it would be too late if the backtrace has already been generated. We could build the backtrace incrementally as recursive calls get unwound, or at least the source locations could be stored incrementally.
This would remove the need to set sf->cur_pc before each call with a potential recursion, but it would not allow for a debugger to explore the call stack from a breakpoint or at the time an exception is thrown.