Re: [PATCH] id,groups: use gidtostr/uidtostr to avoid casts

[Jim Meyering]

Jim Meyering wrote:

> Jim Meyering wrote:
>
>> Jim Meyering wrote:
>>> I wrote this months ago, and like the minimalist (avoiding unnecessary
>>> printf) and more type-safe aspects.  The code-duplication is not ideal,
>>> but the duplicated code is small and fragile, and not generally useful
>>> enough to put in system.h -- too easy to use it from a context where
>>> it would fail.  We could always give it a few more buffer entries, like
>>> human.c does, so two invocations in the same statement won't misbehave.
>>>
>>> Opinions?
>>
>> No response, so I guess it's not horribly to
>> anyone else, either...
>>
>>> Subject: [PATCH] id,groups: use gidtostr/uidtostr to avoid casts
>>>
>>> * src/id.c (gidtostr, uidtostr): Define macros.
>>> (gidtostr_ptr, uidtostr_ptr): Define safer functions.
>>> Use gidtostr and uidtostr to print GID and UID without
>>> need/risk of casts.
>>> * src/group-list.c: Likewise.
>>> ---
>>>  src/group-list.c |   16 ++++++++++++----
>>>  src/id.c         |   38 +++++++++++++++++++++++++++++---------
>>>  2 files changed, 41 insertions(+), 13 deletions(-)
>> ...
>>
>> I rebased and was about to push it, but discovered that tests were failing:
>> when compiled with -O2 and with today's (from-svn-this-morning) gcc,
>>
>>     gcc version 4.8.0 20120516 (experimental) (GCC)
>>
>> it segfaults.  It works fine when compiled with -O1, or with F17's
>>
>>     gcc (GCC) 4.7.0 20120504 (Red Hat 4.7.0-4)
>>
>> Whee...  While trying to debug that, I managed to trigger
>> a failed assertion in gdb.  First priority is to report that.
>
> Reported here:
>
>     http://sourceware.org/bugzilla/show_bug.cgi?id=14119
>
>> I'll hold off until I know more...
>
> When you write 9 bytes past the end of a stack buffer,
> you should expect bad things to happen.  The type used
> as the INT_BUFSIZE_BOUND argument must match the *tostr
> function that uses that buffer.
>
> Yet another reason I'm glad to use cutting-edge gcc.
>
> I'm squashing this in:
>
> diff --git a/src/id.c b/src/id.c
> index d060a94..1f99b9f 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -280,7 +280,7 @@ main (int argc, char **argv)
>  static char *
>  gidtostr_ptr (gid_t const *gid)
>  {
> -  static char buf[INT_BUFSIZE_BOUND (gid_t)];
> +  static char buf[INT_BUFSIZE_BOUND (uintmax_t)];
>    return umaxtostr (*gid, buf);
>  }
>  #define gidtostr(g) gidtostr_ptr (&(g))
> @@ -291,7 +291,7 @@ gidtostr_ptr (gid_t const *gid)
>  static char *
>  uidtostr_ptr (uid_t const *uid)
>  {
> -  static char buf[INT_BUFSIZE_BOUND (uid_t)];
> +  static char buf[INT_BUFSIZE_BOUND (uintmax_t)];
>    return umaxtostr (*uid, buf);
>  }
>  #define uidtostr(u) uidtostr_ptr (&(u))

And the same for the duplicated function (hmm... :-( )
in group-list.c