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.

Sign up for GitHub

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
+3 −3
Merged

Horner degree must be a positive integer #1005

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cafa56d
Horner degree must be a positive integer
schwehr May 19, 2018
fea5387
Horner: fix typo in error message
schwehr May 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/PJ_horner.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,9 +454,9 @@ PJ *PROJECTION(horner) {
/* Polynomial degree specified? */
if (pj_param (P->ctx, P->params, "tdeg").i) { /* degree specified? */
degree = pj_param(P->ctx, P->params, "ideg").i;
if (degree > 10000) {
/* What is a reasonable maximum for the degree? */
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unreasonable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original discussion on the max was here: #893

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return horner_freeup (P, PJD_ERR_INVALID_ARG);
}
} else {
Expand Down