Skip to content
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

Horner degree must be a positive integer #1005

Merged
merged 2 commits into from May 22, 2018
Merged

Conversation

@schwehr
Copy link
Contributor

@schwehr schwehr commented May 19, 2018

Found with autofuzz

Found with autofuzz
@schwehr
Copy link
Contributor Author

@schwehr schwehr commented May 19, 2018

What is the minimum degree for a valid horner 2D polynomial? I presume that maybe that it should fail if < 2 or maybe 1?

https://en.wikipedia.org/wiki/Horner%27s_method

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);

This comment has been minimized.

@QuLogic

QuLogic May 19, 2018
Contributor

unreasonable

This comment has been minimized.

@cffk

cffk May 19, 2018
Contributor

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.

This comment has been minimized.

@schwehr

schwehr May 19, 2018
Author Contributor

@QuLogic Thanks!!!

@cffk:

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:

  1. 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.
  2. There is no warning that you might be requesting a very computationally expensive call. Again, DOS potential
  3. 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).

This comment has been minimized.

@rouault

rouault May 20, 2018
Member

I second Kurt's position: given the current implementation limitations, this improved sanity check is the most reasonable solution.

This comment has been minimized.

@kbevers

kbevers May 20, 2018
Member

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?

This comment has been minimized.

@schwehr

schwehr May 20, 2018
Author Contributor

The original discussion on the max was here: #893

This comment has been minimized.

@kbevers

kbevers May 22, 2018
Member

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.

This comment has been minimized.

@busstoptaktik

busstoptaktik May 22, 2018
Member

... 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...

This comment has been minimized.

@schwehr

schwehr May 22, 2018
Author Contributor

@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.

@kbevers kbevers merged commit 37ebb8f into OSGeo:master May 22, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 77.604%
Details
@kbevers kbevers added the bug label May 22, 2018
@kbevers kbevers added this to the 5.1.0 milestone May 22, 2018
@schwehr schwehr deleted the schwehr:b79669264-horner branch May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.