Bug #9089

rb_fix2uint no longer raises a RangeError when given negative values

Added by Arthur Schreiber over 1 year ago. Updated 12 months ago.

[ruby-core:58207]
Status:Rejected
Priority:Normal
Assignee:-
ruby -v:ruby 2.1.0dev (2013-11-07 trunk 43560) Backport:1.9.3: UNKNOWN, 2.0.0: UNKNOWN

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?

History

#1 Updated by Arthur Schreiber over 1 year 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

#2 Updated by Akira Tanaka over 1 year ago

  • Status changed from Open to Feedback
  • Assignee deleted (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.............

1)
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
'
/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.............

1)
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
'
/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

#3 Updated by Arthur Schreiber over 1 year 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.

#4 Updated by Akira Tanaka over 1 year 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.

#5 Updated by Akira Tanaka over 1 year ago

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

#6 Updated by Hiroshi SHIBATA about 1 year ago

  • Target version changed from 2.1.0 to current: 2.2.0

#7 Updated by Akira Tanaka 12 months 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.

Also available in: Atom PDF