Re: [PATCH] Make grub_strtol() "end" pointers have safer const qualifiers. (v2)

[Daniel Kiper]

On Fri, Feb 21, 2020 at 04:39:33PM -0500, Peter Jones wrote:
> Currently the string functions grub_strtol(), grub_strtoul(), and
> grub_strtoull() don't declare the "end" pointer in such a way as to
> require the pointer itself or the character array to be immutable to the
> implementation, nor does the C standard do so in its similar functions,
> though it does require us not to change any of it.
>
> The typical declarations of these functions follow this pattern:
>
> long
> strtol(const char * restrict nptr, char ** restrict endptr, int base);
>
> Much of the reason for this is historic, and a discussion of that
> follows below, after the explanation of this change.  (GRUB currently
> does not include the "restrict" qualifiers, and we name the arguments a
> bit differently.)
>
> The implementation is semantically required to treat the character array
> as immutable, but such accidental modifications aren't stopped by the
> compiler, and the semantics for both the callers and the implementation
> of these functions are sometimes also helped by adding that requirement.
>
> This patch changes these declarations to follow this pattern instead:
>
> long
> strtol(const char * restrict nptr,
>        const char ** const restrict endptr,
>        int base);
>
> This means that if any modification to these functions accidentally
> introduces either an errant modification to the underlying character
> array, or an accidental assignment to endptr rather than *endptr, the
> compiler should generate an error.  (The two uses of "restrict" in this
> case basically mean strtol() isn't allowed to modify the character array
> by going through *endptr, and endptr isn't allowed to point inside the
> array.)
>
> It also means the typical use case changes to:
>
>   char *s = ...;
>   const char *end;
>   long l;
>
>   l = strtol(s, &end, 10);
>
> Or even:
>
>   const char *p = str;
>   while (p && *p) {
>         long l = strtol(p, &p, 10);
>         ...
>   }
>
> This fixes 26 places where we discard our attempts at treating the data
> safely by doing:
>
>   const char *p = str;
>   long l;
>
>   l = strtol(p, (char **)&ptr, 10);
>
> It also adds 5 places where we do:
>
>   char *p = str;
>   while (p && *p) {
>         long l = strtol(p, (const char ** const)&p, 10);
>         ...
>         /* more calls that need p not to be pointer-to-const */
>   }
>
> While moderately distasteful, this is a better problem to have.
>
> With one minor exception, I have tested that all of this compiles
> without relevant warnings or errors, and that /much/ of it behaves
> correctly, with gcc 9 using 'gcc -W -Wall -Wextra'.  The one exception
> is the changes in grub-core/osdep/aros/hostdisk.c , which I have no idea
> how to build.
>
> Because the C standard defined type-qualifiers in a way that can be
> confusing, in the past there's been a slow but fairly regular stream of
> churn within our patches, which add and remove the const qualifier in many
> of the users of these functions.  This change should help avoid that in
> the future, and in order to help ensure this, I've added an explanation
> in misc.h so that when someone does get a compiler warning about a type
> error, they have the fix at hand.
>
> The reason we don't have "const" in these calls in the standard is
> purely anachronistic: C78 (de facto) did not have type qualifiers in the
> syntax, and the "const" type qualifier was added for C89 (I think; it
> may have been later).  strtol() appears to date from 4.3BSD in 1986,
> which means it could not be added to those functions in the standard
> without breaking compatibility, which is usually avoided.
>
> The syntax chosen for type qualifiers is what has led to the churn
> regarding usage of const, and is especially confusing on string
> functions due to the lack of a string type.  Quoting from C99, the
> syntax is:
>
>  declarator:
>   pointer[opt] direct-declarator
>  direct-declarator:
>   identifier
>   ( declarator )
>   direct-declarator [ type-qualifier-list[opt] assignment-expression[opt] ]
>   ...
>   direct-declarator [ type-qualifier-list[opt] * ]
>   ...
>  pointer:
>   * type-qualifier-list[opt]
>   * type-qualifier-list[opt] pointer
>  type-qualifier-list:
>   type-qualifier
>   type-qualifier-list type-qualifier
>  ...
>  type-qualifier:
>   const
>   restrict
>   volatile
>
> So the examples go like:
>
> const char foo;                       // immutable object
> const char *foo;              // mutable pointer to object
> char * const foo;             // immutable pointer to mutable object
> const char * const foo;               // immutable pointer to immutable object
> const char const * const foo;         // XXX extra const keyword in the middle
> const char * const * const foo; // immutable pointer to immutable
>                               //   pointer to immutable object
> const char ** const foo;      // immutable pointer to mutable pointer
>                               //   to immutable object
>
> Making const left-associative for * and right-associative for everything
> else may not have been the best choice ever, but here we are, and the
> inevitable result is people using trying to use const (as they should!),
> putting it at the wrong place, fighting with the compiler for a bit, and
> then either removing it or typecasting something in a bad way.  I won't
> go into describing restrict, but its syntax has exactly the same issue
> as with const.
>
> Anyway, the last example above actually represents the *behavior* that's
> required of strtol()-like functions, so that's our choice for the "end"
> pointer.
>
> Signed-off-by: Peter Jones <address@hidden>

I went through all the discussions for earlier version and v2 commit
message. Even though it is not perfect solution I think the benefits
outweigh the losses. So, Reviewed-by: Daniel Kiper <address@hidden>
If there are no strong technical objections I will push the patch by the
end of this week.

Daniel