Project

General

Profile

Bug #13232

Comparing BigDecimal to float or Rational fails sometimes

Added by romuloceccon (Rômulo Ceccon) over 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-linux]
[ruby-core:79603]

Description

Under very special cases trying to compare a BigDecimal to a float or Rational will give an unexpected result:

irb> BigDecimal('1') < 1e-10
=> true

I couldn't find a sequence of steps which reproduces the problem with 100% guarantee, but the following seem to get close:

-- install a fresh copy of Ruby 2.3.3 from the stable sources

$ ./configure --prefix=$HOME/ruby-2.3.3-test
$ make main && make install-nodoc
$ export PATH=$HOME/ruby-2.3.3-test/bin:$PATH

-- install active support in $HOME/.gem

$ mkdir $HOME/bug
$ cd $HOME/bug
$ gem install bundler
$ echo -e "source 'https://rubygems.org'\ngem 'activesupport'" > Gemfile
$ bundle install --path=$HOME/.gem

-- run the script:

$ while ruby -e "require 'tempfile'; require 'bigdecimal'; f = Tempfile.new('bug'); srand(1); (1..2000).each { |i| f.rewind && f.puts('test') if i % 1000 == 1; x = BigDecimal('1'); puts([i, x]) || exit(1) if x < 1e-10.to_r }" ; do true ; done

The last script should not return. If it does then the bug is present:

$ while ruby -e ...
962
0.1E1

I can reproduce the bug on Ubuntu 16.04 running with Intel Core i3-4130 (desktop) and on Ubuntu 12.04, 14.04 and 16.04 running with Intel Xeon E5-2651 v2 (AWS).

I could track the problem down to the method VpNewRbClass in bigdecimal.c. It looks like the garbage collector is being called at the wrong time and corrupting memory. Applying the following patch gives me the output below:

--- ext/bigdecimal/bigdecimal.c.orig    2017-02-19 00:30:06.885174089 -0300
+++ ext/bigdecimal/bigdecimal.c 2017-02-19 00:33:53.379501013 -0300
@@ -604,8 +604,14 @@
 VP_EXPORT Real *
 VpNewRbClass(size_t mx, const char *str, VALUE klass)
 {
-    VALUE obj = TypedData_Wrap_Struct(klass, &BigDecimal_data_type, 0);
-    Real *pv = VpAlloc(mx,str);
+    VALUE obj;
+    Real *pv;
+    if (mx == 36)
+        printf("--\nVpNewRbClass: %s\n", str);
+    obj = TypedData_Wrap_Struct(klass, &BigDecimal_data_type, 0);
+    if (mx == 36)
+        printf("VpNewRbClass: %s\n", str);
+    pv = VpAlloc(mx,str);
     RTYPEDDATA_DATA(obj) = pv;
     pv->obj = obj;
     return pv;

Output:

$ while ruby -e ...
....
--
VpNewRbClass: 77371252455336267181195264
VpNewRbClass: 77371252455336267181195264
--
VpNewRbClass: 77371252455336267181195264
VpNewRbClass: 77371252455336267181195264
--
VpNewRbClass: 77371252455336267181195264
VpNewRbClass: ��V
962
0.1E1

Here's a possible fix. Someone please review it, because I have no experience with CRuby and don't feel confident about its correctness:

--- ext/bigdecimal/bigdecimal.c.orig    2017-02-19 00:30:06.885174089 -0300
+++ ext/bigdecimal/bigdecimal.c 2017-02-19 00:39:32.884680313 -0300
@@ -227,6 +227,7 @@
 static Real*
 GetVpValueWithPrec(VALUE v, long prec, int must)
 {
+    ENTER(1);
     Real *pv;
     VALUE num, bg;
     char szD[128];
@@ -292,6 +293,7 @@

       case T_BIGNUM:
         bg = rb_big2str(v, 10);
+        PUSH(bg);
         return VpCreateRbObject(strlen(RSTRING_PTR(bg)) + VpBaseFig() + 1,
                                 RSTRING_PTR(bg));
       default:

Updated by nobu (Nobuyoshi Nakada) over 3 years ago

  • Assignee set to mrkn (Kenta Murata)
  • Status changed from Open to Assigned
  • Description updated (diff)

Seems correct to me.

Updated by romuloceccon (Rômulo Ceccon) over 3 years ago

I didn't realize there was a Github repo for BigDecimal when opening this issue. May I repost it there as a pull request?

Updated by romuloceccon (Rômulo Ceccon) over 3 years ago

Any update on this one?

Should I look for more easily reproducible steps? What about opening a Github issue/PR?

Thanks.

Updated by mrkn (Kenta Murata) over 3 years ago

I'm sorry for my late response.
I'll import your patch by tomorrow.

Thanks a lot.

#5

Updated by mrkn (Kenta Murata) over 3 years ago

  • Status changed from Assigned to Closed

Applied in changeset r57951.


bigdecimal: version 1.3.2

Import bigdecimal version 1.3.2. The full commit log is here:

https://github.com/ruby/bigdecimal/compare/v1.3.1...v1.3.2

This fixes [ruby-core:79603] [Bug #13232]

Updated by hrsht (Harshit Chopra) over 3 years ago

Sorry if it is not the right place to ask this question, but is it possible to backport this fix in ruby 2.4 and 2.3 versions? This issue came to light for us when using ruby 2.4, as with 2.4 BigDecimal now throws an error when trying to initialize with an invalid string.

So for us, it would fail at times when trying to instantiate a BigDecimal object in ruby like:

BigDecimal.new(1.0e-12)

raising error of the form:

invalid value for BigDecimal(): ""
#7

Updated by usa (Usaku NAKAMURA) about 3 years ago

  • Backport changed from 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN to 2.2: WONTFIX, 2.3: REQUIRED, 2.4: REQUIRED

Updated by nagachika (Tomoyuki Chikanaga) over 2 years ago

  • Backport changed from 2.2: WONTFIX, 2.3: REQUIRED, 2.4: REQUIRED to 2.2: WONTFIX, 2.3: REQUIRED, 2.4: DONE

ruby_2_4 r62638 merged revision(s) 57597,57951.

Also available in: Atom PDF