Re: programs using kgio crash under memory pressure during init

[Eric Wong <e@80x24.org>]

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 :)