Horner degree must be a positive integer #1005
Conversation
Found with autofuzz
|
What is the minimum degree for a valid horner 2D polynomial? I presume that maybe that it should fail if |
src/PJ_horner.c
Outdated
| proj_log_debug (P, "Horner: Degree too large: %d", degree); | ||
| if (degree < 0 || degree > 10000) { | ||
| /* What are reasonable minimum and maximums for degree? */ | ||
| proj_log_debug (P, "Horner: Degree is unreasnable: %d", degree); |
There was a problem hiding this comment.
I haven't checked how the routines are coded, but, as a matter of good
software practice, degree = -1 should be allowed and should return 0
(the result of an empty sum). Also, I disapprove of the check on the
upper limit. The polynomial is well defined for arbitrary order and the
code should just carry out the algorithm regardless. If appropriate, an
upper limit should be enforced by the caller.
There was a problem hiding this comment.
@QuLogic Thanks!!!
Thanks for the comments! If I am missing something, please do elaborate on your comments.
I encourage you to submit a follow up commit that allows degree -1 with documentation as to why that is numerically and from a users point of view a good idea. That is IMHO out of scope of this change, which is to block attempts to allocate massive amounts of ram and/or undefined behavior with any negative degree in the existing code. There is no behavior change in my patch beyond not crashes (as far as I can see).
As for a max, this is out of scope for this commit. But, I strongly disagree. Perhaps you can add an option to allow an unlimited degree. But for the default use, unless there is a strong need for extra precision, this is already ridiculously high. There are several issues here with a large degree:
- That causes allocation of large amounts of RAM which is not good in production VMs or in fuzzing. It would then be easy to OOM a small container or DOS a big one.
- There is no warning that you might be requesting a very computationally expensive call. Again, DOS potential
- Is the user really meaning to do request that large of a degree? I am guessing at it will almost always be a mistake.
Why not wait for an actual use case? Then we can work up a solution that doesn't trigger the very real issues from overly large degrees. I can and do get all manner of corrupt data (both accidental and intentional) sent my way to production.
If that max check really does need to go away, please submit a PR where we can discuss more and get a change that doesn't break production systems (aka doesn't allocate massive amounts of ram and doesn't take unreasonable amounts of cycles to complete).
There was a problem hiding this comment.
I second Kurt's position: given the current implementation limitations, this improved sanity check is the most reasonable solution.
There was a problem hiding this comment.
The horner_alloc function expects degree > 0:
/~https://github.com/OSGeo/proj.4/blob/89aeb3d4ccf8683fb10b1a5bea0a5293d2e31817/src/PJ_horner.c#L140-L141
I take that as a hint that it was never the intention to allow a negative polynomial degree. Which I guess doesn't really make any sense either?
Regarding limits, let's ask @busstoptaktik who originally wrote the code and probably has the best feel for what is reasonable and what isn't. What's your take on this, Thomas?
There was a problem hiding this comment.
The original discussion on the max was here: #893
There was a problem hiding this comment.
Just had a brief in person talk with Thomas about this. We both feel that polynomia of degree > 100 are very unlikely to ever be constructed. The current limit though is fine since it leaves plenty of room for even the most ridiculous cases and it cuts of before memory usage gets too extreme.
@schwehr If you remove the comment about what min/max values are reasonable I think this is ready to be merged.
There was a problem hiding this comment.
... note: a polynomium set of degree 10000 will require a few GB of RAM for coefficients, so it may be sensible to reduce the limit a bit. Depending on the setting this may be reasonable or not...
There was a problem hiding this comment.
@busstoptaktik Thanks for the feedback on the max. My fuzzer hasn't hit trouble again on the positive max side yet, but it may well. If you have a number that you think is a reasonable limit, I would be happy to change it in a follow on PR. I'd like to keep this PR just to the lower bounds if possible.
Found with autofuzz