Skip to content

Crash on oom #19

merged 4 commits into from Jun 12, 2012

5 participants

msantos commented May 7, 2012

Some small safety fixes, if anything looks weird or you'd like some changes, please let me know! I've run the tests and everything passed.


@msantos I wish I noticed this PR last week. I fixed a few bugs in erlang_js that were causing segfaults on otp 15B and having never looked at the erlang_js code I didn't know we were so vulnerable to OOM. Thanks for the PR, I'll try and look at this after our next release.

msantos added some commits May 27, 2012
@msantos msantos Remove useless cast from void
Since driver_alloc() returns void, the cast is not required.

Simplify the buffer size calculation as sizeof(char) is defined to be 1.
@msantos msantos Check for OOM conditions
According to the man page for erl_driver, driver_alloc is a wrapper
around malloc():

    This function allocates a memory block of the size specified in size,
    and returns it. This only fails on out of memory, in that case NULL
    is returned. (This  is  most often a wrapper for malloc).

Test for OOM and crash the VM to prevent buffer overflows.

Avoid zeroing buffers that will have their contents overwritten.
@msantos msantos Correct arg passed to JS_DefineFunction
Suppress compiler warning:

c_src/spidermonkey.c: In function ‘sm_initialize’:
c_src/spidermonkey.c:165:21: warning: passing argument 4 of
‘JS_DefineFunction’ from incompatible pointer type [enabled by default]
c_src/system/include/js/jsapi.h:1905:1: note: expected ‘JSNative’ but
argument is of type ‘JSBool (**)(struct JSContext *, struct JSObject *,
        uintN,  jsval *, jsval *)’
@msantos msantos Avoid zeroing buf before copy ad30b0e
msantos commented May 31, 2012

Thanks for looking at it! I've reapplied my patches on top of your changes.

@jaredmorrow jaredmorrow commented on the diff Jun 12, 2012
@@ -240,8 +241,8 @@ char *escape_quotes(char *text) {
char *error_to_json(const spidermonkey_error *error) {
char *escaped_source = escape_quotes(error->offending_source);
/* size = length(escaped source) + length(error msg) + JSON formatting */
- size_t size = (strlen(escaped_source) + strlen(error->msg) + 80) * sizeof(char);
- char *retval = (char *) driver_alloc((ErlDrvSizeT) size);
+ size_t size = strlen(escaped_source) + strlen(error->msg) + 80;
jaredmorrow added a note Jun 12, 2012

* sizeof(char); doesn't seem like it should've been removed

msantos added a note Jun 12, 2012
jaredmorrow added a note Jun 12, 2012

Please ignore me while I confuse sizeof(char) with CHAR_BITS, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chardan chardan was assigned Jun 12, 2012


Basho Technologies member


@jaredmorrow jaredmorrow merged commit 3464e1a into basho:master Jun 12, 2012
chardan commented Jun 12, 2012

+1 Looks good.

It might be nice for maintenance to peek at (driver_comm.c, 37) using the copy_string()
function here instead of the bare code if it's safe (it looks ilke we're still dealing with NULL-terminated strings).

Not created in this change, but copy_string() might be better-named ejs_strdup() to be
more consistent with the standard library in a future pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.