Bug #9089: rb_fix2uint no longer raises a RangeError when given negative values

• Status: Rejected
• Assignee: -
• Target version: 2.2.0
• ruby -v: ruby 2.1.0dev (2013-11-07 trunk 43560)
• Backport: 1.9.3: UNKNOWN, 2.0.0: UNKNOWN

[NoKarma (Arthur Schreiber)]

Up until the change that was made in ((URL:https://github.com/ruby/ruby/commit/92f59c6d7937b14bb5eefb052099ef0a3ef3bcd0)), (({rb_fix2uint})) would raise a (({RangeError})) if it was given a negative value like (({-1})) (e.g. when using the (({FIX2UINT})) macro).

Due to the changes made in that commit, this error is no longer raised and (({rb_fix2uint})) will return an incorrect value instead.

There is a C-API spec in rubyspec that shows that this behavior has changed between 2.0.0-p247 and 2.1.0-preview1, and I checked and made sure this is still not working correctly in the latest 2.1.0-dev version. The failing spec can be found at ((URL:https://github.com/rubyspec/rubyspec/blob/master/optional/capi/fixnum_spec.rb#L16-L18)), it is part of the "optional" c-api specs.

Is there any reason why there is the (({if (num < (unsigned long)INT_MIN)})) is made inside the (({check_uint})) function? Doesn't the (({sign})) parameter automatically indicate that we can't convert to an unsigned integer?

[NoKarma (Arthur Schreiber)]

=begin
I guess I somehow incorrectly formatted the issue description, so here it is again with proper formatting.

---

Up until the change that was made in ((URL:https://github.com/ruby/ruby/commit/92f59c6d7937b14bb5eefb052099ef0a3ef3bcd0)), (({rb_fix2uint})) would raise a (({RangeError})) if it was given a negative value like (({-1})) (e.g. when using the (({FIX2UINT})) macro).

Due to the changes made in that commit, this error is no longer raised and (({rb_fix2uint})) will return an incorrect value instead.

There is a C-API spec in rubyspec that shows that this behavior has changed between 2.0.0-p247 and 2.1.0-preview1, and I checked and made sure this is still not working correctly in the latest 2.1.0-dev version. The failing spec can be found at ((URL:https://github.com/rubyspec/rubyspec/blob/master/optional/capi/fixnum_spec.rb#L16-L18)), it is part of the "optional" c-api specs.

Is there any reason why there is the (({if (num < (unsigned long)INT_MIN)})) is made inside the (({check_uint})) function? Doesn't the (({sign})) parameter automatically indicate that we can't convert to an unsigned integer?

=end

[akr (Akira Tanaka)]

• Status changed from Open to Feedback
• Assignee deleted (akr (Akira Tanaka))

As far as I know, NUM2Uxxx accepts negative integers.
For consistency, FIX2Uxxx should behaves so.

Also, both r40028 and r40029 behaves same for
r40028/bin/ruby rubyspec/optional/capi/fixnum_spec.rb.

So r40029 doesn't change the behavior.

% svn log -r r40029

r40029 | akr | 2013-04-01 12:06:09 +0900 (Mon, 01 Apr 2013) | 4 lines

• numeric.c (check_uint): Take the 1st argument as unsigned long,
  instead of VALUE. Refine the validity test conditions.
  (check_ushort): Ditto.

---

% ./r40028/bin/ruby -v
ruby 2.1.0dev (2013-04-01 trunk 40028) [x86_64-linux]
% ./r40029/bin/ruby -v
ruby 2.1.0dev (2013-04-01 trunk 40029) [x86_64-linux]

% ruby mspec/bin/mspec -t r40028/bin/ruby rubyspec/optional/capi/fixnum_spec.rb
ruby 2.1.0dev (2013-04-01 trunk 40028) [x86_64-linux]
.F.............

CApiFixnumSpecs rb_fix2uint raises a RangeError when given a negative value FAILED
Expected RangeError but no exception was raised (-1 was returned)
/home/akr/fix2int/mspec/lib/mspec/expectations/expectations.rb:15:in fail_with' /home/akr/fix2int/mspec/lib/mspec/expectations/should.rb:8:in should'
/home/akr/fix2int/rubyspec/optional/capi/fixnum_spec.rb:17:in block (4 levels) in <top (required)>' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in instance_eval'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in protect' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in block in protect'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in each' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in all?'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in protect' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:208:in block (2 levels) in process'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:239:in block in repeat' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:238:in times'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:238:in repeat' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:200:in block in process'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:199:in each' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:199:in process'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in block in process' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in each'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in process' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:38:in describe'
/home/akr/fix2int/mspec/lib/mspec/runner/object.rb:11:in describe' /home/akr/fix2int/rubyspec/optional/capi/fixnum_spec.rb:5:in <top (required)>'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in load' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in block (2 levels) in files'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in instance_eval' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in protect'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in block in files' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:51:in each'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:51:in files' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:43:in process'
/home/akr/fix2int/mspec/lib/mspec/commands/mspec-run.rb:94:in run' /home/akr/fix2int/mspec/lib/mspec/utils/script.rb:218:in main'
/home/akr/fix2int/mspec/bin/mspec-run:8:in `'

Finished in 0.003072 seconds

1 file, 15 examples, 17 expectations, 1 failure, 0 errors
% ruby mspec/bin/mspec -t r40029/bin/ruby rubyspec/optional/capi/fixnum_spec.rb
ruby 2.1.0dev (2013-04-01 trunk 40029) [x86_64-linux]
.F.............

CApiFixnumSpecs rb_fix2uint raises a RangeError when given a negative value FAILED
Expected RangeError but no exception was raised (-1 was returned)
/home/akr/fix2int/mspec/lib/mspec/expectations/expectations.rb:15:in fail_with' /home/akr/fix2int/mspec/lib/mspec/expectations/should.rb:8:in should'
/home/akr/fix2int/rubyspec/optional/capi/fixnum_spec.rb:17:in block (4 levels) in <top (required)>' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in instance_eval'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in protect' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in block in protect'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in each' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in all?'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:179:in protect' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:208:in block (2 levels) in process'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:239:in block in repeat' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:238:in times'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:238:in repeat' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:200:in block in process'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:199:in each' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:199:in process'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in block in process' /home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in each'
/home/akr/fix2int/mspec/lib/mspec/runner/context.rb:230:in process' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:38:in describe'
/home/akr/fix2int/mspec/lib/mspec/runner/object.rb:11:in describe' /home/akr/fix2int/rubyspec/optional/capi/fixnum_spec.rb:5:in <top (required)>'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in load' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in block (2 levels) in files'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in instance_eval' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:69:in protect'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:57:in block in files' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:51:in each'
/home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:51:in files' /home/akr/fix2int/mspec/lib/mspec/runner/mspec.rb:43:in process'
/home/akr/fix2int/mspec/lib/mspec/commands/mspec-run.rb:94:in run' /home/akr/fix2int/mspec/lib/mspec/utils/script.rb:218:in main'
/home/akr/fix2int/mspec/bin/mspec-run:8:in `'

Finished in 0.003090 seconds

1 file, 15 examples, 17 expectations, 1 failure, 0 errors

[NoKarma (Arthur Schreiber)]

That's weird. If you go back to the previous change that was made in numeric.c, the fix2uint specs do pass:

Arthurs-iMac-2:rubyspec arthur$ ../mspec/bin/mspec optional/capi/fixnum_spec.rb
ruby 2.1.0dev (2013-03-31 trunk 40017) [x86_64-darwin13.0.0]
...............

Finished in 0.002706 seconds

1 file, 15 examples, 17 expectations, 0 failures, 0 errors
Arthurs-iMac-2:rubyspec arthur$

Also, I'd argue that the existing FIX2UINT behavior was 'correct'. From the perspective of writing a Ruby Extension, it makes no sense that negative values passed to FIX2UINT result in an overflow. E.g. I was using it to call a native method that expected an unsigned int. If a user now passes a negative value from Ruby, He'll get an absolutely confusing result.

[akr (Akira Tanaka)]

NoKarma (Arthur Schreiber) wrote:

That's weird. If you go back to the previous change that was made in numeric.c, the fix2uint specs do pass:

Arthurs-iMac-2:rubyspec arthur$ ../mspec/bin/mspec optional/capi/fixnum_spec.rb
ruby 2.1.0dev (2013-03-31 trunk 40017) [x86_64-darwin13.0.0]
...............

If so, something between r40017 and r40028 changes the behavior.

Also, I'd argue that the existing FIX2UINT behavior was 'correct'. From the perspective of writing a Ruby Extension, it makes no sense that negative values passed to FIX2UINT result in an overflow. E.g. I was using it to call a native method that expected an unsigned int. If a user now passes a negative value from Ruby, He'll get an absolutely confusing result.

Other conversions such as NUM2SHORT(-1), NUM2UINT(-1), NUM2ULONG(-1), NUM2ULL(-1), FIX2ULONG(-1) doesn't raise an exception.
I think it is confusing that only FIX2UINT(-1) raises an exception.

[akr (Akira Tanaka)]

One more thing:
FIX2UINT(-1) doesn't raise an exception on 32bit environment on Ruby 2.0.

[hsbt (Hiroshi SHIBATA)]

• Target version changed from 2.1.0 to 2.2.0

[akr (Akira Tanaka)]

• Status changed from Feedback to Rejected

This issue is not acceptable because it is very inconsistent that only FIX2UINT on 64bit environment rejects negative numbers.
Changing the behavior of other functions, NUM2Uxxx and FIX2Uxxx, is too incompatible.

If people needs rigid conversion functions, it is better to define them as new functions.
Also, rb_integer_pack is usable to convert numbers and detect overflow and sign.

[irxground (Daichi Ota)]

I felt uncomfortable with the behavior of rb_num2ull. I don't know if it is correct to comment on this ticket, but it seemed to be similar.

The current behavior of rb_num2ll is as follows.

range	behavior
val < -2^63	raise an error
-2^63 <= val < 2^63	correct value
2^63 < val	raise an error

The behavior of rb_num2ull is as follows.

range	behavior
val < -2^63	raise an error
-2^63 <= val < 0	overflow
0 <= val < 2^64	correct value
2^64 < val	raise an error

I think it's inconsistent, so it's nice to fix it.