WIP: issue #241: range checking #242
Conversation
|
@tbeu something like this? |
|
Yes, with setting |
|
Please let me also know if UBSan is happy with this kind of fixes. |
| #include <stddef.h> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
| #include <stdint.h> | |
| #include <limits.h> |
This fails if HAVE_STDINT_H is not defined. Did you mean to include limits.h instead?
There was a problem hiding this comment.
Oh. Doesn't matio require C99? stdint.h I believe is the correct header for UINT32_MAX and friends: https://en.cppreference.com/w/c/types/integer
There was a problem hiding this comment.
No, it's not C99. Go for https://en.cppreference.com/w/c/types/limits instead.
There was a problem hiding this comment.
No, it's not C99.
Oh. So I guess that's why you have mat_int32_t instead of just using int32_t?
Go for https://en.cppreference.com/w/c/types/limits instead.
UINT32_MAX is not there.
So I guess we need a custom MAT_UINT32_MAX somewhere...
There was a problem hiding this comment.
Or I just use literals like 4294967295U?
There was a problem hiding this comment.
You can see the safe-math.h where literals (in hex) are used.
src/read_data.c
Outdated
| TT min_ = (TT)READ_TYPE_MIN; \ | ||
| TT max_ = (TT)READ_TYPE_MAX; \ | ||
| const size_t block_size = READ_BLOCK_SIZE / data_size; \ | ||
| if ( len <= block_size ) { \ | ||
| readcount = fread(v, data_size, len, (FILE *)mat->fp); \ | ||
| if ( readcount == len ) { \ | ||
| for ( i = 0; i < len; i++ ) { \ | ||
| data[i] = (T)v[i]; \ | ||
| TT val_ = v[i]; \ | ||
| if (val_ >= min_ && val_ <= max_) { \ | ||
| data[i] = (T)val_; \ | ||
| } else { \ | ||
| break; \ | ||
| } \ |
There was a problem hiding this comment.
So I've just realized a problem... when this cast on line 57 is doing float -> int, let's say uint8_t specifically, the valid range of floats that can be legally cast to uint8_t are [-0.5, 255.5] (I forget if inclusive or exclusive), but READ_TYPE_MIN is 0 and READ_TYPE_MAX is 255, so we would in fact start rejecting some valid cases.
Not sure best way to do this...
There was a problem hiding this comment.
For integer conversion you can read http://tzimmermann.org/2018/04/20/safe-integer-conversion-in-c/.
For floating-point to integer conversion we need to deal with rounding. There is https://en.cppreference.com/w/c/numeric/math/round in C99, which rounds to nearest integer.
There was a problem hiding this comment.
For integer conversion you can read http://tzimmermann.org/2018/04/20/safe-integer-conversion-in-c/.
It's a nice write up, thanks for sharing. I'm familiar with these thing already though. Here's another nice write up you might like: https://www.frama-c.com/2013/05/02/Harder-than-it-looks-rounding-float-to-nearest-integer-part-1.html It's tricky stuff!
For floating-point to integer conversion we need to deal with rounding. There is https://en.cppreference.com/w/c/numeric/math/round in C99, which rounds to nearest integer.
The current behaviour in master (data[i] = (T)v[i];) does not round, it truncates. Do you want to change that?
But what I meant by "Not sure best way to do this" was more with regards to this codebase, not numeric conversions in C generally. The macros like READ_DATA maybe need to be split into 2 macros, one for integers and one for floats. Consider your suggestion of calling round: how can READ_DATA conditionally do that when it doesn't know if it's dealing with integers or floats? Maybe I'm missing some nice way... typeof might be handy, but that's new in C23...
There was a problem hiding this comment.
@tbeu want me to just take my best stab at it?
There was a problem hiding this comment.
I gave it a try, but have just realised what I've committed is rather wrong... will continue tomorrow.
tbeu
force-pushed
the
master
branch
6 times, most recently
from
bba9fd4
to
fe6c885
Compare
for more information, see https://pre-commit.ci
tbeu
force-pushed
the
master
branch
2 times, most recently
from
8c0e212
to
1e5c40e
Compare
tbeu
force-pushed
the
master
branch
9 times, most recently
from
e19ab9d
to
25b818e
Compare
tbeu
force-pushed
the
master
branch
4 times, most recently
from
0df6711
to
c335ecb
Compare
No description provided.