From: Markus Date: 2004-08-31T03:22:36+09:00 Subject: (Revised) [PATCH] Subtle bug in bignum.c On Sat, 2004-08-28 at 08:25, writes: > > t> It's really subtle :-) > > really, really subtle :-) > > ... > > ...it will call bignew() which will call rb_gc(), but in ebx it has > RBIGNUM(x)->digits. This mean that when ruby will mark the registers it > will not mark the Bignum created by rb_big_clone() and the GC remove the > Bignum created. Guy -- That makes sense (and I'd agree with the "really, really subtle assessment). So although my original patch "fixes" the problem, it does so in a way that is not obvious. To that end, I'd suggest (remember, I am NOT a C programmer; this compiles and it works, but that's all I can claim for it) something along the lines of: --- ruby-1.8.1.org/bignum.c 2003-12-22 02:25:49.000000000 -0700 +++ ruby-1.8.1/bignum.c 2004-08-30 11:06:33.000000000 -0700 @@ -1615,6 +1615,8 @@ VALUE x, y; { VALUE z; + VALUE *y_gc = NULL; + VALUE *x_gc = NULL; BDIGIT *ds1, *ds2, *zds; long i, l1, l2; char sign; @@ -1624,11 +1626,13 @@ y = rb_int2big(FIX2LONG(y)); } if (!RBIGNUM(y)->sign) { - y = rb_big_clone(y); + y = rb_big_clone(y); + rb_gc_register_address(x_gc = &y); get2comp(y, Qtrue); } if (!RBIGNUM(x)->sign) { - x = rb_big_clone(x); + x = rb_big_clone(x); + rb_gc_register_address(x_gc = &x); get2comp(x, Qtrue); } if (RBIGNUM(x)->len > RBIGNUM(y)->len) { @@ -1655,6 +1659,8 @@ zds[i] = sign?0:ds2[i]; } if (!RBIGNUM(z)->sign) get2comp(z, Qfalse); + if (y_gc) rb_gc_unregister_address(y_gc); + if (x_gc) rb_gc_unregister_address(x_gc); return bignorm(z); } @@ -1670,6 +1676,8 @@ VALUE x, y; { VALUE z; + VALUE *y_gc = NULL; + VALUE *x_gc = NULL; BDIGIT *ds1, *ds2, *zds; long i, l1, l2; char sign; @@ -1681,10 +1689,12 @@ if (!RBIGNUM(y)->sign) { y = rb_big_clone(y); + rb_gc_register_address(x_gc = &y); get2comp(y, Qtrue); } if (!RBIGNUM(x)->sign) { x = rb_big_clone(x); + rb_gc_register_address(x_gc = &x); get2comp(x, Qtrue); } if (RBIGNUM(x)->len > RBIGNUM(y)->len) { @@ -1711,6 +1721,8 @@ zds[i] = sign?ds2[i]:(BIGRAD-1); } if (!RBIGNUM(z)->sign) get2comp(z, Qfalse); + if (y_gc) rb_gc_unregister_address(y_gc); + if (x_gc) rb_gc_unregister_address(x_gc); return bignorm(z); } ...which may not be ideal but at least is clearer in intent. In the process of working up this patch, I began to wonder: how do we know that other operations are not similarly afflicted? It would seem (IMHO) that relying on temporaries to be findable in registers (if I understand you correctly) would be fraught with opportunities for this sort of errors--especially since the gc could be called by deeply nested code that has no inkling of the use to which it being put. This could also (again, if I'm understanding correctly) affect code other than bignum--in fact, any code that makes multiple temporary objects without registering them in some way. Is this something we can/should do something about beyond dealing with Bignum.& and Bignum.|? -- Markus On Sat, 2004-08-28 at 08:25, writes: > > t> It's really subtle :-) > > really, really subtle :-) > > Breakpoint 4, rb_big_and (x=1074241584, y=1074241584) at bignum.c:1637 > 1637 y = rb_to_int(y); > (gdb) info register ebx > ebx 0x4007a030 1074241584 > (gdb) printf "0x%x\n", x > 0x4007a030 > (gdb) > > the variable `x' is in ebx > > 1647 get2comp(x, Qtrue); > (gdb) info register ebx > ebx 0x4007a008 1074241544 > (gdb) printf "0x%x\n", x > 0x4007a008 > (gdb) p *(struct RBignum *)x > $8 = {basic = {flags = 13, klass = 1074419244}, sign = 0 '\0', len = 1, > digits = 0x8117870} > (gdb) > > it create a new Bignum in rb_big_clone(), `x' is still in ebx > > 1663 z = bignew(l2, RBIGNUM(x)->sign || RBIGNUM(y)->sign); > (gdb) info register ebx > ebx 0x8117700 135362304 > (gdb) > > it will call bignew() which will call rb_gc(), but in ebx it has > RBIGNUM(x)->digits. This mean that when ruby will mark the registers it > will not mark the Bignum created by rb_big_clone() and the GC remove the > Bignum created. > > > > Guy Decoux