GNOME Bugzilla – Bug 707131
Why the arbitrary MAX_SIZE in pango/pango-markup.c?
Last modified: 2013-09-04 01:10:29 UTC
Hi, in pango/pango-markup.c, a "size" attribute has a maximum value. This value is arbitrarily defined this way: /* cap size from the top at an arbitrary 2048 */ #define MAX_SIZE (2048 * PANGO_SCALE) I checked the comment of commit 3d03fdbf which introduced this behavior but there was not explanation on this specific limitation. There are usually good technical reasons for this kind of limit (int max size on the platform, etc.) but here this value seems completely random. Moreover "n" is set by pango_scan_int() which uses strtol() internally (pango/pango-utils.c) and thus will return the appropriate error anyway if there is an int overflow during parsing. In other words, it looks to me that this MAX_SIZE may be useless and redundant, no? I would thus propose to remove this MAX_SIZE. Nevertheless if we really want to keep such a value (maybe for some valid reason I can't see), I'd say it should be a proper pango macro (maybe PANGO_MAX_FONT_SIZE set in pango/pango-font.h?) so that an external program can try and catch the error before, rather than trying to set, and fail.
Created attachment 253625 [details] [review] Patch to get rid of arbitrary MAX_SIZE, replace by INT_MAX Here is a proposal patch. I don't change much of the code, simply I use INT_MAX as the maximum font size. At least this is a value which makes sense, not random, since I see the font size is stored as int.
I don't remember why I added that. Feel free to push.
commit e8d1c3f32b38cbd4e39d389ba103af33302bf01a Author: Jehan <jehan@girinstud.io> Date: Sat Aug 31 03:02:41 2013 +1200 Bug 707131 - getting rid of the arbitrary MAX_SIZE for font. Since the value is stored as an int, no reason to use any other value than INT_MAX.
Correct me if I'm wrong, but isn't 'n>INT_MAX' for 'int n' a bogus check ? '(unsigned)n>INT_MAX' does make sense, not the other, but 'n' *is* signed.
Hi, well yeah considering that pango_scan_int() will only returns an int. So right now, that's definitely bogus. But pango_scan_int() uses strtol() internally (returns long int, which has a bigger max than int on many platforms), even though the current implementation would convert this long return to int and thus return FALSE to a too big value. So it would be very easy to change the implementation to get a long instead. My idea is that if it ever changes (because someone thinks that what can do more can do less), well there should be a value test ready to not end up in a situation where the spec is different than the implementation. So I just stayed on the safe side by explicitly writing down the bogus test. But yeah ultimately you are right. We can remove the (n > INT_MAX) test. If the maintainer wants, I can remove it and maybe also add a comment? Something like this: diff --git a/pango/pango-markup.c b/pango/pango-markup.c index 2b2c304..13ce01a 100644 --- a/pango/pango-markup.c +++ b/pango/pango-markup.c @@ -1190,7 +1190,13 @@ span_parse_func (MarkupData *md G_GNUC_UNUSED, const char *end; gint n; - if ((end = size, !pango_scan_int (&end, &n)) || *end != '\0' || n < 0 || n > INT_MAX) + /* pango_scan_int() returns FALSE if the scanned value was over + * INT_MAX; and the reference out value's type is an int. Thus a + * (n > INT_MAX) test is unecessary. + * If pango_scan_int() behavior was to be changed to accept long + * int, an explicit test should be added (or the PangoAttrSize + * type updated to handle long int too). */ + if ((end = size, !pango_scan_int (&end, &n)) || *end != '\0' || n < 0)
No need for the comment. Just cleanup the code please.
Done. No comment in-code. commit 285be5bd7ee3ce87bb027b405be674d6f91995d1 Author: Jehan <jehan@girinstud.io> Date: Wed Sep 4 13:04:47 2013 +1200 Bug 707131 - removing useless test. Since pango_scan_int() would return FALSE if the scanned value was over INT_MAX; and the reference out value's type is an int, (n > INT_MAX) test is unecessary. If pango_scan_int() behavior was to be changed to accept long int, an explicit test should be added (or the PangoAttrSize type updated to handle long int too).