-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fix multiple bugs in IO::Buffer.map and update its documentation #15264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5268e70 to
ede2a1d
Compare
|
The test failure doesn't seem connected with my changes in any way and didn't happen before fixing style.
|
|
|
||
| rb_off_t file_size = rb_file_size(io); | ||
| // Compiler can confirm that we handled file_size <= 0 case: | ||
| if (UNLIKELY(file_size <= 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (UNLIKELY(file_size <= 0)) { | |
| if (UNLIKELY(file_size < 0)) { |
In wonder if we should allow mapping zero-sized files? Not sure what the point would be, but is it technically invalid?
| rb_raise(rb_eArgError, "Invalid negative or zero file size!"); | ||
| } | ||
| // Here, we assume that file_size is positive: | ||
| else if (UNLIKELY((uintmax_t)file_size > SIZE_MAX)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it only matters if the mapped size is going to be too big (bigger than SIZE_MAX).
Also, I believe RB_NUM2SIZE will already raise if out of range, e.g.:
unsigned long
rb_big2ulong(VALUE x)
{
unsigned long num = big2ulong(x, "unsigned long");
if (BIGNUM_POSITIVE_P(x)) {
return num;
}
else {
if (num <= 1+(unsigned long)(-(LONG_MIN+1)))
return -(long)(num-1)-1;
}
rb_raise(rb_eRangeError, "bignum out of range of unsigned long");
}
- Buffer's size did not account for offset when mapping the file, leading to possible crashes. - Size and offset were not checked properly, leading to many situations raising Errno errors with generic messages. - Documentation was wrong.
ede2a1d to
6f2472f
Compare
ioquatix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly agree with this change. I may make some follow up refinements once this is merged.
Main issues that are fixed:
Other issues were about not checking arguments enough, leading to a proliferation of
Errno::EINVALwith generic messages:I've also added a bunch of tests. Note, however, that there are no tests for successfully using
offset, as it's impossible to reliably use (see https://bugs.ruby-lang.org/issues/21700).