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 <pjo...@redhat.com>
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 <daniel.ki...@oracle.com>
If there are no strong technical objections I will push the patch by the
end of this week.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
