From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-2.8 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, URIBL_BLOCKED shortcircuit=no autolearn=unavailable version=3.3.2 X-Original-To: kgio-public@bogomips.org Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 320E51FA6C; Tue, 30 Dec 2014 07:22:25 +0000 (UTC) Date: Tue, 30 Dec 2014 07:22:25 +0000 From: Eric Wong To: Petr Novodvorskiy Cc: kgio-public@bogomips.org, Lara Martin , Nick Astete Subject: Re: programs using kgio crash under memory pressure during init Message-ID: <20141230072225.GA17690@dcvr.yhbt.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: Petr Novodvorskiy 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 :)