Bug #9089
closed
rb_fix2uint no longer raises a RangeError when given negative values
Description
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?
Updated by NoKarma (Arthur Schreiber) over 10 years ago
=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
Updated by akr (Akira Tanaka) over 10 years ago
- 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
Updated by NoKarma (Arthur Schreiber) over 10 years ago
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.
Updated by akr (Akira Tanaka) over 10 years ago
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.
Updated by akr (Akira Tanaka) over 10 years ago
One more thing:
FIX2UINT(-1) doesn't raise an exception on 32bit environment on Ruby 2.0.
Updated by hsbt (Hiroshi SHIBATA) about 10 years ago
- Target version changed from 2.1.0 to 2.2.0
Updated by akr (Akira Tanaka) almost 10 years ago
- 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.
Updated by irxground (Daichi Ota) almost 5 years ago
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.