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
ut_encode_clock overflow when fuzzing #70
Comments
I can't think of a good solution to this because the function is publicly visible and doesn't return a status value. Look like I painted myself into a corner. The documentation for this function will state that the absolute values of the arguments must lie within certain ranges. |
I don't think this is resolved. The documentation changes are definitely good, but you still have a case where user input can trigger
All of the above are ABI compatible, but do cause behavior changes. However, the current implementation is not stable. The compiler can do what it pleases with values that overflow a signed int. Compilers are sometimes triggering |
#1 isn't backward compatible. The function's been published and we must assume it's being used. #2.a Is a possibility. though I'm not sure that it's better than the documentation changes. "Must" means must -- either way you get something back that's meaningless. #2.b Negative return values from ut_encode_clock() are valid and the clock encoding isn't specified, so choosing a value is problematic. BTW, the behavior might be unspecified, but it's not undefined (i.e., it won't delete all you files and crash). |
This function most definitely has undefined behahavior and some compilers will raise sigill... Which is allowed by the C spec. They also can do anything else they want :( Please explain your reasoning for 1 with double literals. What exactly isn't backward compat? |
Changing a published API, in general, isn't backward compatible -- although in this case going from integer to double really isn't much of a change. I'm not sure what it would get you, however, as you could still call the function with values that might cause a SIGFPE. |
I am confused... The return type stays double. There may be some slight varations with the resulting double, but there are no unit tests that break, yes? If there are cases that might break if the results of this function suffer from slight floating point precision issues, there really needs to be at least one test case |
Ah... when you commit changes that relate to an issue, can you please reference the issue in the commit message? Then it will automatically appear in the issue's timeline. e.g. 42ae946 relates to this issue |
@schwehr Even if the hour argument were a As far as test-cases go, in an ideal world, with unlimited time and budget, I'd agree. What we have here, however, is an oversight on my part that's unlikely to be a real problem but does get dinged by an automated test system. I can live with that. |
Ok. I can't live with it. I will have to put a custom hack into my system or the fuzzer will fall all over this. Users can and do provide all sorts of crazy input for a variety of reasons. It gets super hard to track down when it happens in large scale production systems. udunits is getting used in satellite processing code and I can't predict how things will go sideways. The seconds being a double does leave room for a floating point overflow and I hadn't noticed that. For completeness, it's when seconds approaches Fuzzers are great at providing test input that cover cases like this. Once I get udunits setup, I will get bug reports with "proof of concept" (poc) files that contain the input that triggered undefined behavior. |
I use the POSIX standard myself. One possibility would be for I'll put that in. |
It's pretty easy to overflow the signed integer math in
ut_encode_clock()
. One way to fix this would be to use double constants as the multiplier. 60 -> 60.0. But it might be better to range check the hours, minutes, and seconds args. The code might issue a warning if things are out of a reasonable time range and either clamp the result or return 0.0.Also, that is the expected range of hours, minutes and seconds? Should it be okay with a leap second? Is this return seconds since midnight and on what day is this relative to? Can hours be greater than 25? Can seconds be 60?
The text was updated successfully, but these errors were encountered: