valgrind should detect reading off the end of argv

NEW
Unassigned

Status

()

Core
JavaScript Engine
7 years ago
3 years ago

People

(Reporter: mrbkap, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
We've had examples where code forgot to check argc before reading a slot from argv. In these cases, anything can happen, from finding a "double" to finding an old object that happened to have been left on the stack. It appears that because the memory that we pass to native functions is just in the middle of the contiguous stack, it's likely to be in the middle of a block of memory that is otherwise "allocated" but hasn't been set-up for the function call.

Would it be possible to add annotations or something so that valgrind would see these reads as being reading uninitialized memory?

dvander also suggested that this might be something to throw static analysis at instead of valgrind.
You could mark the memory just after argv as inaccessible with Valgrind, yes.  Question is, what is that memory?  Is it something that's accessed?  If so, that'll lead to false positive errors.

Comment 2

7 years ago
At all times there is an exact top-of-stack (in the general case, evaluated by StackSpace::firstUnused).  Usually its just cx->regs->sp.  Rather than trying to notify valgrind on every push/pop (hard), perhaps we could have a mechanism for telling valgrind the address of a variable which acts as the top of a stack?  That way there would only be a handful of places to update; mostly StackSpace/JSContext memfuns.

Updated

7 years ago
Blocks: 636940
(Reporter)

Comment 3

7 years ago
How worried are we about the interpreter/a JIT reading off the end of the stack? Do you think it'd be worth simply marking the end of the stack when calling into natives/getters/setters and then expanding the end of the stack when we return?
(Assignee)

Updated

3 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.