Avoid RangeError on integers larger than LONG_LONG #764

nurse · 2016-06-28T10:10:48Z

CRuby's rb_big2ull raises RangeError if the argument is out of [LONG_MIN, ULONG_MAX].
To avoid that calling to_s and set as decimal with some tricks for edge cases.

sodabrew · 2016-06-28T14:27:46Z

spec/mysql2/statement_spec.rb

+    int64_min = -(1 << 63)
+    result = stmt.execute(int64_max, int64_min)
+    expect(result.to_a).to eq(['max' => int64_max, 'min' => int64_min])
+    @client.query 'DROP TABLE IF EXISTS mysql2_stmt_q'


No need to drop the table since these tests aren't creating/querying from it.

sodabrew · 2016-06-28T14:46:32Z

Thank you! Will this also work in the future merged Integer / FixNum / BigNum world of Ruby 2.4.0?

brianmario#764 (comment)

nurse · 2016-06-29T05:31:35Z

Yeah, TYPE(v) == T_BIGNUM still works with Ruby 2.4.

sodabrew · 2016-06-29T06:48:04Z

Ruby 1.8.7 does have rb_big_cmp, but it is static: http://rxr.whitequark.org/mri/source/bignum.c?v=1.8.7-p374#1107

You could call Bignum#<=> to get the same effect?

/* elsewhere in the file */
static ID id_cmp;
id_cmp = rb_intern("<=>");
/**/

#ifdef HAVE_RB_BIG_CMP
  VALUE less_than_llong_min = rb_big_cmp(bignum, LL2NUM(LLONG_MIN));
#else
  VALUE less_than_llong_min = rb_funcall(bignum, id_cmp, 1, LL2NUM(LLONG_MIN));
#endif
  if (less_than_llong_min == INT2FIX(-1)) {
    // code
  } else {
    // code
  }

nurse · 2016-06-29T10:23:38Z

rb_funcall can be an alternative but it is slow.

I noticed len == 8 && nlz_bits == 0 && rb_absint_singlebit_p(bignum) is the best condition because it doesn't allocate extra object.
For older rubies I'm using rb_big_and or rb_big_cmp now, but rb_funcall may be better for the people who maintains the code. I'll fix them if you prefer that.

kamipo · 2016-06-29T11:05:38Z

ext/mysql2/statement.c

+#endif
+    if (len > 8) goto overflow;
+    if (RBIGNUM_POSITIVE_P(bignum)) {
+        num = rb_big2ull(bignum);


Looks like if BIGSIZE(bignum) > SIZEOF_LONG_LONG, should be goto overflow.
http://rxr.whitequark.org/mri/source/bignum.c#3587

Oh, enough to if (len > 8) goto overflow;?

Yes. len > 8 means "Bignum's raw size is larger than 8 byte.
It is the same meaning with BIGSIZE(bignum) > SIZEOF_LONG_LONG.

Anyway the spec should have int64_max3 = 1 << 64; I'll add it tomorrow.

sodabrew · 2016-07-07T05:55:39Z

ext/mysql2/statement.c

+#else
+  len = RBIGNUM_LEN(bignum) * SIZEOF_BDIGITS;
+#endif
+  if (len > 8) goto overflow;


Length 8 is intended to be the same as sizeof(long long int) right? Would it make sense for the integer size to be passed in as an argument?

brianmario#764 (comment)

nurse · 2016-07-20T06:45:22Z

@sodabrew I reflected your comments. What do you think about current one?

sodabrew · 2016-10-18T20:17:39Z

Sorry for the long delay on this PR, and it's run into a merge conflict since the recent ssl_mode PR added other items to extconf.rb. Do you have time to rebase this PR? If not I can do a rebase and squash merge mysql.

CRuby's rb_big2ull raises RangeError if the argument is out of [LONG_MIN, ULONG_MAX]. To avoid that calling to_s and set as decimal with some tricks for edge cases. For Ruby 1.8.7, use rb_funcall() to call Bignum#<=> through Ruby space. For Ruby 1.9.3 and 2.0.0, use rb_big_cmp() to call Bignum#<=> directly. For Ruby 2.1 and above, use rb_absint_size() and I noticed len == 8 && nlz_bits == 0 && rb_absint_singlebit_p(bignum) is the best condition because it doesn't allocate extra object.

nurse · 2016-10-19T05:32:01Z

Hmm, rebased but it fails. I'll investigate later...

sodabrew · 2016-10-19T05:53:11Z

Thanks! There's an interaction with #783 that results in the values getting cast as BigDecimal instead of a string on return. The reason is that MySQL defaults to returning numeric values as decimals unless it knows a specific integer size from a column definition or a procedure signature.

Reminds me of #563 and #557 and #58.

sodabrew · 2016-10-19T06:16:30Z

I think #783 actually correctly fixed the big number types, and you can remove the .to_s that look like a workaround in the test?

diff --git a/spec/mysql2/statement_spec.rb b/spec/mysql2/statement_spec.rb
index 517bd31..3d00535 100644
--- a/spec/mysql2/statement_spec.rb
+++ b/spec/mysql2/statement_spec.rb
@@ -78,7 +78,7 @@ RSpec.describe Mysql2::Statement do
     int64_min2 = -(1 << 64) + 1
     int64_min3 = -0xC000000000000000
     result = stmt.execute(int64_max1, int64_max2, int64_max3, int64_min1, int64_min2, int64_min3)
-    expect(result.to_a).to eq(['max1' => int64_max1.to_s, 'max2' => int64_max2.to_s, 'max3' => int64_max3.to_s, 'min1' => int64_min1.to_s, 'min2' => int64_min2.to_s, 'min3' => int6
+    expect(result.to_a).to eq(['max1' => int64_max1, 'max2' => int64_max2, 'max3' => int64_max3, 'min1' => int64_min1, 'min2' => int64_min2, 'min3' => int64_min3])
   end

* Use rb_absint_size and rb_absint_singlebit_p on Ruby 2.1 or later. They don't allocate new objects and fast. * Use rb_big_cmp on Ruby 1.9.x and 2.0.0 * Use rb_funcall(bignum, rb_intern("<=>")) on Ruby 1.8 and REE Note that this works on Ruby 2.4.

nurse · 2016-10-19T10:42:15Z

@sodabrew Fixed as you says removed to_s. It still fails on some environments but seems not related with this PR...

sodabrew reviewed Jun 28, 2016
View reviewed changes

kamipo mentioned this pull request Jun 28, 2016

Drop Ruby 1.8.7 and REE support #766

Closed

nurse force-pushed the dont-raise-rangeerror-for-large-integer branch from f47cb50 to 1a0703c Compare June 29, 2016 04:46

nurse added a commit to nurse/mysql2 that referenced this pull request Jun 29, 2016

write normal case before with more readable condition

0aacab3

brianmario#764 (comment)

kamipo reviewed Jun 29, 2016
View reviewed changes

sodabrew added this to the 0.4.5 milestone Jun 29, 2016

nurse changed the title ~~[WIP] Avoid RangeError on integers larger than LONG_LONG~~ Avoid RangeError on integers larger than LONG_LONG Jul 7, 2016

sodabrew reviewed Jul 7, 2016
View reviewed changes

nurse force-pushed the dont-raise-rangeerror-for-large-integer branch 2 times, most recently from 37410c4 to 81fb72b Compare July 7, 2016 06:24

nurse added a commit to nurse/mysql2 that referenced this pull request Jul 7, 2016

write normal case before with more readable condition

0c4f0f7

brianmario#764 (comment)

nurse force-pushed the dont-raise-rangeerror-for-large-integer branch from c2c67e7 to f59eb53 Compare October 19, 2016 04:49

nurse force-pushed the dont-raise-rangeerror-for-large-integer branch from f59eb53 to f10a337 Compare October 19, 2016 10:03

sodabrew merged commit 5e2b3a6 into brianmario:master Oct 19, 2016

nurse deleted the dont-raise-rangeerror-for-large-integer branch October 19, 2016 14:39

kamipo mentioned this pull request Mar 2, 2017

Prepared statement with bignum primary key fails #828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid RangeError on integers larger than LONG_LONG #764

Avoid RangeError on integers larger than LONG_LONG #764

nurse commented Jun 28, 2016 •

edited

Loading

sodabrew Jun 28, 2016

sodabrew commented Jun 28, 2016

nurse commented Jun 29, 2016

sodabrew commented Jun 29, 2016

nurse commented Jun 29, 2016 •

edited

Loading

kamipo Jun 29, 2016

kamipo Jun 29, 2016

nurse Jun 29, 2016 •

edited

Loading

sodabrew Jul 7, 2016

nurse Jul 7, 2016

nurse commented Jul 20, 2016

sodabrew commented Oct 18, 2016

nurse commented Oct 19, 2016

sodabrew commented Oct 19, 2016 •

edited

Loading

sodabrew commented Oct 19, 2016

nurse commented Oct 19, 2016

Avoid RangeError on integers larger than LONG_LONG #764

Avoid RangeError on integers larger than LONG_LONG #764

Conversation

nurse commented Jun 28, 2016 • edited Loading

sodabrew Jun 28, 2016

Choose a reason for hiding this comment

sodabrew commented Jun 28, 2016

nurse commented Jun 29, 2016

sodabrew commented Jun 29, 2016

nurse commented Jun 29, 2016 • edited Loading

kamipo Jun 29, 2016

Choose a reason for hiding this comment

kamipo Jun 29, 2016

Choose a reason for hiding this comment

nurse Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

sodabrew Jul 7, 2016

Choose a reason for hiding this comment

nurse Jul 7, 2016

Choose a reason for hiding this comment

nurse commented Jul 20, 2016

sodabrew commented Oct 18, 2016

nurse commented Oct 19, 2016

sodabrew commented Oct 19, 2016 • edited Loading

sodabrew commented Oct 19, 2016

nurse commented Oct 19, 2016

nurse commented Jun 28, 2016 •

edited

Loading

nurse commented Jun 29, 2016 •

edited

Loading

nurse Jun 29, 2016 •

edited

Loading

sodabrew commented Oct 19, 2016 •

edited

Loading