programs using kgio crash under memory pressure during init

programs using kgio crash under memory pressure during init

[Petr Novodvorskiy]

In rare cases when we run our unit tests we get following error:

/var/lib/gems/1.8/gems/kgio-2.7.4/lib/kgio_ext.so: [BUG] constant not a
symbol or string
ruby 1.8.7 (2011-06-30 patchlevel 352) [x86_64-linux]

Aborted (core dumped)
rake aborted!

I was able to run out unit tests in memory constrained environment and get
to reproduce it 4 out of 5 runs. I was able to reproduce it even after I
upgraded our kgio version.

After looking in the code, I think I was able to identify the problem. In
init_kgio_tryopen(), when rb_funcall is called to get list of errno
constants, it doesn't protect returned list from garbage collecting. I made
modification (below) that seemed to fix the problem (I wasn't able to
reproduce it anymore).

Thanks,
Petr.

diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c
index 20f3f6d..d97a5fa 100644
--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -162,7 +162,13 @@ void init_kgio_tryopen(void)
                rb_define_alias(cFile, "to_path", "path");

        errno2sym = st_init_numtable();
-       tmp = rb_funcall(rb_mErrno, rb_intern("constants"), 0);
+
+        VALUE *get_err_constants(VALUE blah) {
+          return rb_funcall(rb_mErrno, rb_intern("constants"), 0);
+        }
+        int exception;
+        tmp = rb_protect(get_err_constants, (VALUE)NULL, &exception);
+
        len = RARRAY_LEN(tmp);
        for (i = 0; i < len; i++) {
                VALUE error;

Re: programs using kgio crash under memory pressure during init

[Eric Wong]

Petr Novodvorskiy <nidd@skytap.com> wrote:
> In rare cases when we run our unit tests we get following error:
> 
> /var/lib/gems/1.8/gems/kgio-2.7.4/lib/kgio_ext.so: [BUG] constant not a
> symbol or string
> ruby 1.8.7 (2011-06-30 patchlevel 352) [x86_64-linux]
> 
> Aborted (core dumped)
> rake aborted!
> 
> I was able to run out unit tests in memory constrained environment and get
> to reproduce it 4 out of 5 runs. I was able to reproduce it even after I
> upgraded our kgio version.
> 
> After looking in the code, I think I was able to identify the problem. In
> init_kgio_tryopen(), when rb_funcall is called to get list of errno
> constants, it doesn't protect returned list from garbage collecting. I made
> modification (below) that seemed to fix the problem (I wasn't able to
> reproduce it anymore).

Good catch!

I think the following fix is sufficient to add a RB_GC_GUARD at the end.
Can you give the following a shot?  I also have another small patch to
try below...

--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -194,4 +194,5 @@ void init_kgio_tryopen(void)
 			          ID2SYM(const_id));
 		}
 	}
+	RB_GC_GUARD(tmp);
 }

> 
> Thanks,
> Petr.
> 
> diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c
> index 20f3f6d..d97a5fa 100644
> --- a/ext/kgio/tryopen.c
> +++ b/ext/kgio/tryopen.c
> @@ -162,7 +162,13 @@ void init_kgio_tryopen(void)
>                 rb_define_alias(cFile, "to_path", "path");
> 
>         errno2sym = st_init_numtable();
> -       tmp = rb_funcall(rb_mErrno, rb_intern("constants"), 0);
> +
> +        VALUE *get_err_constants(VALUE blah) {
> +          return rb_funcall(rb_mErrno, rb_intern("constants"), 0);
> +        }
> +        int exception;
> +        tmp = rb_protect(get_err_constants, (VALUE)NULL, &exception);
> +

I actually don't think your patch sufficient in all cases and a compiler
may still optimize tmp away without the RB_GC_GUARD I proposed above.

Also, declaring functions inside other functions is not very portable,
as is declaring variables after statements.

However, an even more surgical fix for 1.8 users only may be:

--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -170,8 +170,11 @@ void init_kgio_tryopen(void)
 		ID const_id;
 
 		switch (TYPE(err)) {
-		case T_SYMBOL: const_id = SYM2ID(err); break;
-		case T_STRING: const_id = rb_intern(RSTRING_PTR(err)); break;
+		case T_SYMBOL: const_id = SYM2ID(err); break; /* Ruby 1.9+ */
+		case T_STRING: /* Ruby 1.8 only */
+			const_id = rb_intern(RSTRING_PTR(err));
+			RB_GC_GUARD(err);
+			break;
 		default: {
 			VALUE i = rb_inspect(err);
 			const char *s = RSTRING_PTR(i);

My first patch was broader and maybe that's needed.

Fwiw, I've actually been thinking about dropping Ruby 1.8 support, but
it seems like you guys are still using it, so I'll consider keeping it.
(Most Rubyists have long dropped 1.8, though).

Thanks again for reporting the bug.  One of these patches should fix it :)

Re: programs using kgio crash under memory pressure during init

[Hleb Valoshka]

On 12/30/14, Petr Novodvorskiy <nidd@skytap.com> wrote:
> After looking in the code, I think I was able to identify the problem. In
> init_kgio_tryopen(), when rb_funcall is called to get list of errno
> constants, it doesn't protect returned list from garbage collecting. I made
> modification (below) that seemed to fix the problem (I wasn't able to
> reproduce it anymore).

> +        tmp = rb_protect(get_err_constants, (VALUE)NULL, &exception);

I don't know why does it help, but rb_protect is wrong function for
your purpose:
VALUE rb_protect(VALUE (*func) (VALUE), VALUE arg, int *state)

    Calls the function func with arg as the argument. If no exception
occured during func, it returns the result of func and *state is zero.
Otherwise, it returns Qnil and sets *state to nonzero. If state is
NULL, it is not set in both cases. You have to clear the error info
with rb_set_errinfo(Qnil) when ignoring the caught exception.

Re: programs using kgio crash under memory pressure during init

[Eric Wong]

Btw, did you guys get a chance to test the changes I proposed?  Thanks.

[PATCH] tryopen: add RB_GC_GUARD for Ruby 1.8

[Eric Wong]

Hi Petr, I haven't heard from you, but I'm nearly certain adding
one RB_GC_GUARD will solve your problem and will push out the
following patch, thanks.

--------------------------8<----------------------------
From: Eric Wong <e@80x24.org>
Subject: [PATCH] tryopen: add RB_GC_GUARD for Ruby 1.8

Calling rb_intern(RSTRING_PTR(err)) was unsafe as the `err'
value could be GC-ed inside rb_intern.

This only affected Ruby 1.8 as Errno.constants returns an
array of Symbols under 1.9+

Thanks to Petr Novodvorskiy for reporting the issue.

ref: <CAHGF7iJk9-wK5WbSiAgQaRye7y2RJttLBpykRBpnKkxFi=ASHg@mail.gmail.com>
Cc: Petr Novodvorskiy <nidd@skytap.com>
Cc: Lara Martin <lmartin@skytap.com>
Cc: Nick Astete <nastete@skytap.com>
---
 ext/kgio/tryopen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ext/kgio/tryopen.c b/ext/kgio/tryopen.c
index 20f3f6d..d87cb17 100644
--- a/ext/kgio/tryopen.c
+++ b/ext/kgio/tryopen.c
@@ -194,4 +194,5 @@ void init_kgio_tryopen(void)
 			          ID2SYM(const_id));
 		}
 	}
+	RB_GC_GUARD(tmp);
 }
-- 
EW