[SSSD] [PATCHES] Enable printf format string checking

Jakub Hrozek jhrozek at redhat.com
Mon Sep 2 22:25:09 UTC 2013


On Wed, Aug 21, 2013 at 03:35:11PM +0200, Lukas Slebodnik wrote:
> On (21/08/13 13:46), Jakub Hrozek wrote:
> >Sorry it took so long before we got to another review of the patch. The
> >ticket is targeted for 1.12 so initially I was thinking of the patchset
> >as lower priority than the other work.
> >
> >On Wed, Jul 24, 2013 at 10:32:29AM +0200, Lukas Slebodnik wrote:
> >> On (24/07/13 09:41), Ondrej Kos wrote:
> >> >On 07/23/2013 06:21 PM, Lukas Slebodnik wrote:
> >> >>ehlo,
> >> >>
> >> >>It would be great to have enabled printf format string checking in RHEL7.
> >> >>Therefore I decided to send pateches for ticket
> >> >>https://fedorahosted.org/sssd/ticket/1945
> >> >>
> >> >>Patch 0001 -- Even if this patch is first, it should be applied in upstream as
> >> >>          last.
> >
> >              ^^^^ Does it matter? I'll apply the whole set anyway..
> It can make a big noise in case of bisecting.
> I moved this patch to the end.

Thanks. I'll try to remember to ask someone to double-check before pushing
:-)

> 
> >
> >Some comments below..
> >
> >> >>
> >> >>Patch 0002 - 0006 -- trivial
> >> >>Patch 0007 -- ssize_t is not defined in ANSI c99, IIRC it is defined in posix
> >> >>               as a signed type, therefore format should be used "zd"
> >> >>Patch 0008 -- size_t is defined in ANSI c99 as unzigned type -> "zu"
> >
> >btw size_t seems to be part of standard library, not the language
> >specification per se. Not that it matters for the patch :)
> size_t can be part of standard library, but ANSI c99 says size_t is unsigned
> type.
> 
> Problem is that sizeof(size_t) is different on 32-bit platform and 64-bit
> platform

OK

> 
> >
> >> >>Patch 0009 -- wrapper for inttypes.h and for future format macros.
> >> >>Patch 0012 -- formating types defined in stdint.h uint_32_t ...
> >> >>            -- there are used macros defined in inttypes.h
> >> >>Patches 0010 - 0015 fix formating for special variables (key_serial_t, rlim_t...)
> >> >>                     and for some types I created macros in sss_format.h
> >> >>Patch 0010 -- key_serial_t is typedef of int32_t, but it could be defined
> >> >>          differently in another (platforms/ implemantations of kerberos)
> >> >>Patch 0011 -- it seems that rlim_t us the same as uint64_t, but it was defined
> >> >>            using conditional build an not a typedef of std types.
> >> >>Patch 0013 -- time_t is defined as "long int" sizeof_i386(time_t) != sizeof_x86_64(time_t)
> >> >>Patch 0014 -- ber_int and ber_tag are typedef for int and unsigned long
> >> >>Patch 0015 -- gid_t and uid_t are typedef as unsigned
> >> >>               (I checked linux 32 bit, linu 64_bit and freebsd 64 bit)
> >
> >Actually /usr/include/bits/typesizes.h says:
> >#define __UID_T_TYPE            __U32_TYPE
> and you forgot to add:
> shell$ grep -RniI __U32_TYPE
> bits/types.h:101:#define __U32_TYPE             unsigned int

I think this is lucky coincidence where unsigned int is 32bits wide.

But to be honest I haven't found any information that would state that
uid_t or gid_t is guaranteed to be 32bit (even though some places in the
SSSD rely on uid_t == uint32_t). As far as I can see, POSIX simply says
that both are opaque numeric types. Too bad the system headers don't
provide some kind of format specifier..

I wonder if we should check the width during configure and define our
own specifier based on what we discover?

> 
> >
> >(and then defines __UID_T_TYPE to __uid_t which in turn gets typedefed to uid_t)
> >

[snip]

> >> Subject: [PATCH 10/16] Fix formating of variables with type: key_serial_t
> >> Subject: [PATCH 11/16] Fix formating of variables with type: rlim_t
> >> Subject: [PATCH 12/16] Fix formating of variables with type defined in stdint.h
> >
> >Nice, I didn't know these existed. But to be honest, these format specifiers
> >seem ugly to me..wouldn't it be nicer to cast the variables to a larger type
> >(long or unsigned long) and then use plan C conversion?
> I agree with Stephen casting is not very portable.
> Some platforms needn't have defned long long unsigend or there can be another
> type which is bigger.
> 
> uintmax_t/intmax_t could be used as the larget type.
> In this case modifier "j" should be used
>     format_fun("%jd %ju\n" (uintmax_t) uint_8_var, (intmax_t)int8_t_var)
> but it is overkill to cast uint_8_var to 64_vit variable (or in future 128 bit:-)
> 
> And last point. I don't like casting of argumets in formating functions.
> 
> ------------------
> "man inttypes.h" is very helpful
> ------------------

OK, I'm convinced :-)

> 
> >> Subject: [PATCH 13/16] Fix formating of variables with type: time_t
> >I think this is OK, my C reference says that time_t can be of "any arithmetic
> >type" but also states that long is traditionally used.
> >
> >> Subject: [PATCH 14/16] Fix formating of variables with ber_ type
> >ACK
> >
> >> Subject: [PATCH 15/16] Fix formating of variables with type: [gu]?id_t
> >
> >This is the main point of discussion in the patchset I think. IIRC POSIX
> >states that IDs are 32bit unsigned integers. That's precisely why we have
> >casted them up to unsigned (long) long. Did you see warnings with some
> >strict combination of CFLAGS?
> >
> -Wformat is automatically  enabled by -Wall, There is no problem with CFLAGS.
> You can try to revert this patch (or do not apply this patch)
> And warnings will appear. clang gives more hints.
> 
> But it is possibel to add another macro to header file sss_format.h
> 
> >> Subject: [PATCH 16/16] Fix warning: data argument not used by format string
> >Oof, these are bad. Thank you. ACK.
> Those fixes are not critical, only unsufficient information will be printed.
> 
> Updated patches are attached.
> 
> LS

Sorry the review has stalled for a bit. I think that once we resolve the
uid_t patch, we can push the whole patchset.



More information about the sssd-devel mailing list