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

Improved numerical validation. #6458

Merged
merged 28 commits into from
Jan 26, 2022
Merged

Improved numerical validation. #6458

merged 28 commits into from
Jan 26, 2022

Conversation

sdlime
Copy link
Member

@sdlime sdlime commented Jan 24, 2022

Implements a common approach to validating numerical values read from the mapfile at parse time. Meant to complement runtime validation done throughout the code. Review and additions most welcome.

mapfile.c Outdated
if((symbol = getSymbol(2, MS_NUMBER,MS_BINDING)) == -1) return(-1);
if(symbol == MS_NUMBER) {
label->shadowsizex = (int) msyynumber;
label->shadowsizex = (int) msyynumber; // TODO > 0
Copy link
Member

Choose a reason for hiding this comment

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

Are these TODOs due to complications around adding in the checks in these cases?
In similar cases below they are commented "ok?"

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is an oversight on my part - forgot to resolve the "TODO". Will update...

@@ -4751,31 +4785,32 @@ int loadReferenceMap(referenceMapObj *ref, mapObj *map)
if(loadColor(&(ref->outlinecolor), NULL) != MS_SUCCESS) return(-1);
break;
case(SIZE):
if(getInteger(&(ref->width)) == -1) return(-1);
if(getInteger(&(ref->height)) == -1) return(-1);
if(getInteger(&(ref->width), MS_NUM_CHECK_RANGE, 5, ref->map->maxsize) == -1) return(-1); // is 5 reasonable?
Copy link
Member

Choose a reason for hiding this comment

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

Should the minimum values of 5 be added to https://mapserver.org/mapfile/reference.html#mapfile-reference-size ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Once there is agreement that that's the appropriate value. It seemed reasonable to me...

@@ -6176,7 +6213,7 @@ static int loadMapInternal(mapObj *map)
break;
case(ANGLE): {
double rotation_angle;
if(getDouble(&(rotation_angle)) == -1) return MS_FAILURE;
if(getDouble(&(rotation_angle), MS_NUM_CHECK_RANGE, -360, 360) == -1) return MS_FAILURE;
Copy link
Member

Choose a reason for hiding this comment

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

Can a https://mapserver.org/mapfile/map.html#mapfile-map-angle MAP->ANGLE have a negative value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so - I mean it makes sense to me. @tbonfort, what do you think?

{
querymap->map = (mapObj *)map;
Copy link
Member

Choose a reason for hiding this comment

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

Could there be any issues with adding the map as a property to the querymap? Would it be safer to pass in the maxsize and minsize values directly to this function?

Copy link
Member Author

@sdlime sdlime Jan 24, 2022

Choose a reason for hiding this comment

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

We use the same approach with other objects (e.g. layer, scalebar) where there is an opaque pointer back to the parent so this should be ok I'd think so I was sticking to that pattern. Maybe @rouault has an opinion on this practice.

@@ -6528,7 +6561,7 @@ int msUpdateMapFromURL(mapObj *map, char *variable, char *string)
msyystring = string;
msyylex();

if(getDouble(&(rotation_angle)) == -1) break;
if(getDouble(&(rotation_angle), MS_NUM_CHECK_RANGE, -360, 360) == -1) break;
Copy link
Member

Choose a reason for hiding this comment

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

As above, I was unaware you could use negative angles (to presumably set anti-clockwise rotation?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be good to get confirmation from @tbonfort but this should be ok and should be easy to test.

#ifdef SWIG
%immutable;
#endif /* SWIG */
struct mapObj *map; ///< Reference to parent :class:`mapObj`
Copy link
Member

Choose a reason for hiding this comment

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

May not be required if only used to get the min and max size when loading the map (see comment above).

#define MS_SCALEBAR_INTERVALS_MIN 1
#define MS_SCALEBAR_INTERVALS_MAX 100

#define MS_SCALEBAR_WIDTH_MIN 5
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, once values are confirmed.

@@ -1517,6 +1537,11 @@ The :ref:`REFERENCE <reference>` object
/* legendObj */
/************************************************************************/

#define MS_LEGEND_KEYSIZE_MIN 5
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, once values are confirmed.

@geographika
Copy link
Member

@sdlime - looks good to me, and any validation that can be done at the Mapfile parsing stage will save a lot of pain later on.

I ran on a couple of large Mapfiles I have here, and hit the error message loadStyle(): General error message. Invalid WIDTH, must be greater than 0. On further investigation this was correctly thrown as I had a STYLE with a WIDTH 0 which serves no purpose.

CLASS
..
            STYLE
                COLOR 0 0 0
                WIDTH 0
            END
            STYLE
                COLOR 255 192 203
                WIDTH 4
            END
        END
END

Do you expect parsing speeds to be impacted in any way? I can try and test that here to see.

Finally I can look at making a docs pull request with the min/max values for various keywords if that helps.

@sdlime sdlime requested a review from rouault January 25, 2022 17:38
@sdlime
Copy link
Member Author

sdlime commented Jan 25, 2022

@geographika, I'm not anticipating any significant impact on parsing speed. If you have the opportunity to validate then that would be great. In the big scheme of things additions like the config file will have a more significant affect - especially for straight CGI execution.

In terms of things like WIDTH 0, I wrestled with handling default values when I worked through this. I did do some testing when writing mapfiles via MapScript to make sure that default values weren't being written routinely. I mean, we wouldn't want MapServer to barf reading a mapfile that was written by MapServer. That doesn't seem to be the case here and we can recommend commenting things out rather than using a value like 0.

That said, we could conditionally allow default values in a few cases if something like WIDTH 0 is pretty commonplace - recommended in the docs or whatever.

--Steve

mapfile.c Show resolved Hide resolved
…d clarifying comments in loadColor(). Resolved a couple of TODO comments.
@sdlime
Copy link
Member Author

sdlime commented Jan 26, 2022

I think I got things resolved (except for docs)... Please let me know about merging. --Steve

@rouault rouault self-requested a review January 26, 2022 13:46
@sdlime
Copy link
Member Author

sdlime commented Jan 26, 2022

This will be a candidate for backport to 7.6 but I think we should wait until some 8.0 testing happens.

@sdlime sdlime merged commit e74a176 into MapServer:main Jan 26, 2022
@geographika
Copy link
Member

@sdlime - do you want me to update the docs? I'm also going to update mappyfile to include the min/max values so can do both at the same time.

@sdlime
Copy link
Member Author

sdlime commented Jan 26, 2022

@sdlime - do you want me to update the docs? I'm also going to update mappyfile to include the min/max values so can do both at the same time.

That would be great!

@jmckenna jmckenna added this to the 8.0 Release milestone Jan 26, 2022
@jmckenna
Copy link
Member

re: backport: I'd also prefer getting feedback through main branch first (will do tests with MS4W today)

re: docs: I'd like to see us getting back to filing an associated ticket in the documentation repository, for each new feature: the benefit is that the docs team will test each new feature, while adding the docs. This also is a way to keep track of what is missing in the docs.

@ejn
Copy link
Contributor

ejn commented Jan 26, 2022

As a user I'd say that this change (much as I find it a good thing (TM)) shouldn't be backported - it's a change that could break existing installations. Yes, those are installations with "invalid" mapfiles, but if they weren't sufficiently invalid that they couldn't be used then it would be rude to introduce what is effectively a breaking change in a minor (or even bugfix) release.

@sdlime sdlime deleted the inv branch January 26, 2022 14:21
@jmckenna
Copy link
Member

I agree with you @ejn

@jmckenna
Copy link
Member

jmckenna commented Jan 26, 2022

@sdlime something bizarre is happening locally for me, sorry if this sounds like a crazy question: could the output of map2img's -map_debug switch be affected by these changes? -all_debug 3 works fine. -map_debug 3 generates the map image but does not output any debug messages (just a blank line). This works with a build from a month ago. Something is different, but I don't know what yet. (either in my build, or a change in the code)

@sdlime
Copy link
Member Author

sdlime commented Jan 26, 2022

No, the various debug settings are unaffected. This is actually an oversight since if debug is given as a number then it should be checked as well but the code in mapfile.c uses getSymbol() so I missed it. I can patch that...

Anyway, map2img is setting map->debug directly, apart from what's in the mapfile so these changes have no bearing on that. I'm happy to try on my end if you have a test case.

@jmckenna
Copy link
Member

Ok thanks, makes sense. I'll take a deeper look and see if it was something on my end (likely) first.

@jmckenna
Copy link
Member

@sdlime solved, it was on my end. Will continue testing this...

@jmckenna
Copy link
Member

@Steve can something be done to include a line number in the error message? For example, a mapfile with 30 entries of WIDTH 0 returns only the following with map2img:

loadStyle(): General error message. Invalid WIDTH, must be greater than 0
loadStyle(): General error message. Invalid WIDTH, must be greater than 0 <br>

Or is this a huge can of worms, for another day?

@sdlime
Copy link
Member Author

sdlime commented Jan 27, 2022

@Steve can something be done to include a line number in the error message? For example, a mapfile with 30 entries of WIDTH 0 returns only the following with map2img:

loadStyle(): General error message. Invalid WIDTH, must be greater than 0
loadStyle(): General error message. Invalid WIDTH, must be greater than 0 <br>

Or is this a huge can of worms, for another day?

No, this can be addressed in the error handling. That said, we could also test for >= 0 if width=0 is a common setting (the default width is 1). @geographika has a ticket #6463 that is documenting some exceptions we'll need to address. Perhaps you could add this there so it's all in one spot.

sdlime added a commit to sdlime/mapserver that referenced this pull request Oct 6, 2022
* Refactor validation of numeric content read from the mapfile.
* Updated create image function with guard to ensure positive image size (matches Cairo approach).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants