svaarala / duktape

Duktape - embeddable Javascript engine with a focus on portability and compact footprint
MIT License
5.93k stars 514 forks source link

Avoid longjmp() between va_start() and va_end() #1791

Open svaarala opened 6 years ago

svaarala commented 6 years ago

There are a few such cases in the code base right now. Depending on the varargs implementation, that might leak memory.

Because almost anything non-trivial may try to allocate, and thus throw an out-of-memory, in many cases a varargs call may need to use a safe call so that the the matching va_end() can be guaranteed to execute before a rethrow.

fatcerberus commented 6 years ago

Varargs are really fun in cases involving noreturn functions (e.g. duk_error_va()). The compiler warns for unreachable code on the va_end() even though everything I've read says you should always call va_end anyway.

svaarala commented 6 years ago

Yes, duk_error_va() is ultimately not a very good API call because it has some portability limitations; namely that if va_start() performs allocations and missing a va_end() means a memory leak, one shouldn't really call it. But then it'd maybe be better if it didn't exist at all. In any case the API call documentation should mention this.

A better idea is to push an error object with a varargs argument, call va_end(), and then throw the result. But even that's pretty tricky: what if there's no value stack space when pushing the error? The caller has then called va_start() and we can't throw - but neither can we satisfy the value stack contract. So the error pusher would need to have a boolean indicating whether the push succeeded or not, so that the caller could va_end(), and then decide what to do. That decision isn't easy if one really just wants to throw an error... what to do if you can't even push one? :) (Sure, that's mismanagement of the value stack, but still.)

One reason va_end() should always be in place regardless of whether it's reached is also because sometimes va_start() / va_end() can expand to a { ... } delimited block, so it may be a syntax error to omit the va_end().