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..
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 :)
>>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 then defines __UID_T_TYPE to __uid_t which in turn gets typedefed to uid_t)
>>Patch 0016 -- trivial
>>
>>LS
>>
>>
>Hi,
>
>I'm looking at the patches now, and I see, that somewhere you changed
>the debug levels to the new style and somewhere you left the numeric
>value. Could you please replace the debug levels with new style
>anywhere you're modifying code?
>
>Ondra
>
Thank you for review.
I didn't fix warnings at once, because there were a lot of warnings 180+.
So it seems, that I changed debug levels only sometimes.
Updated patches are attached.
LS
From ee2cfd16d12e11d4878d8137b958e797bc2228a3 Mon Sep 17 00:00:00
2001
From: Lukas Slebodnik <lslebodn(a)redhat.com>
Date: Thu, 23 May 2013 13:14:37 +0200
Subject: [PATCH 01/16] Enable printf format string checking
https://fedorahosted.org/sssd/ticket/1945
---
configure.ac | 18 ++++++++++++++++++
src/util/util.h | 14 ++++++++++----
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac
index b9b72e3d7cd3adf1bd127e6ae9546a460ee0055e..abde94774fb3d4e60302ff74ad4e9829550cfcc7
100644
--- a/configure.ac
+++ b/configure.ac
@@ -264,6 +264,24 @@ if test x"$sss_client_cv_attribute_destructor" = xyes ;
then
[whether compiler supports __attribute__((destructor))])
fi
+AC_CACHE_CHECK([whether compiler supports __attribute__((format))],
+ sss_client_cv_attribute_format,
+ [AC_COMPILE_IFELSE(
+ [AC_LANG_SOURCE(
+ [void debug_fn(const char *format, ...) __attribute__ ((format
(printf, 1, 2)));]
+ )],
+ [sss_client_cv_attribute_format=yes],
Can you remove the _client_ part? The variable is not used only in
client..
+ [
+ AC_MSG_RESULT([no])
+ AC_MSG_WARN([compiler does NOT support
__attribute__((format))])
+ ])
+ ])
+
+if test x"$sss_client_cv_attribute_format" = xyes ; then
+ AC_DEFINE(HAVE_FUNCTION_ATTRIBUTE_FORMAT, 1,
+ [whether compiler supports __attribute__((format))])
+fi
+
PKG_CHECK_MODULES([CHECK], [check >= 0.9.5], [have_check=1], [have_check=])
if test x$have_check = x; then
AC_MSG_WARN([Without the 'CHECK' libraries, you will be unable to run all
tests in the 'make check' suite])
diff --git a/src/util/util.h b/src/util/util.h
index 5acc67bec7ee94b85963d04f5bfcca017cf87abd..c328f7e9fb6d4bbd8667d171ccd0e593d1f0e70f
100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -54,13 +54,19 @@
#define CLEAR_MC_FLAG "clear_mc_flag"
+#ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT
+#define SSS_ATTRIBUTE_PRINTF(a1, a2) __attribute__ ((format (printf, a1, a2)))
+#else
+#define SSS_ATTRIBUTE_PRINTF(a1, a2)
+#endif
+
extern const char *debug_prg_name;
extern int debug_level;
extern int debug_timestamps;
extern int debug_microseconds;
extern int debug_to_file;
extern const char *debug_log_file;
-void debug_fn(const char *format, ...);
+void debug_fn(const char *format, ...) SSS_ATTRIBUTE_PRINTF(1, 2);
int debug_get_level(int old_level);
int debug_convert_old_level(int old_level);
errno_t set_debug_file_from_fd(const int fd);
@@ -128,7 +134,7 @@ errno_t set_debug_file_from_fd(const int fd);
memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \
__debug_macro_datetime[19] = '\0'; \
if (debug_microseconds) { \
- debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): ", \
+ debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): ", \
This change seems correct to me, but can you split it to a separate
patch? I'd like to have only the autoconf macro in the new patch.
__debug_macro_datetime,
__debug_macro_tv.tv_usec, \
__debug_macro_year, debug_prg_name, \
__FUNCTION__, __debug_macro_newlevel); \
@@ -169,7 +175,7 @@ errno_t set_debug_file_from_fd(const int fd);
memcpy(__debug_macro_datetime, ctime(&__debug_macro_tv.tv_sec), 19); \
__debug_macro_datetime[19] = '\0'; \
if (debug_microseconds) { \
- debug_fn("(%s:%.6d %d) [%s] [%s] (%#.4x): %s\n", \
+ debug_fn("(%s:%.6ld %d) [%s] [%s] (%#.4x): %s\n", \
__debug_macro_datetime, __debug_macro_tv.tv_usec, \
__debug_macro_year, debug_prg_name, \
function, __debug_macro_newlevel, message); \
@@ -292,7 +298,7 @@ void talloc_log_fn(const char *msg);
#define SSS_LOG_INFO 6 /* informational */
#define SSS_LOG_DEBUG 7 /* debug-level messages */
-void sss_log(int priority, const char *format, ...);
+void sss_log(int priority, const char *format, ...) SSS_ATTRIBUTE_PRINTF(2, 3);
/* from server.c */
struct main_context {
--
1.8.3.1
Subject: [PATCH 02/16] Fix formating of variables with type: long
Subject: [PATCH 03/16] Fix formating of variables with type: unsigned long
Subject: [PATCH 04/16] Fix formating of variables with type: int
Subject: [PATCH 05/16] Fix pointer formatting
Subject: [PATCH 06/16] Use the same variable type like in struct
ACK, trivial
patches
Subject: [PATCH 07/16] Fix formating of variables with type: ssize_t
ACK. I think it's quite standard practice to use zd for ssize_t, but I
haven't found a canonical source in my C books either.
Subject: [PATCH 08/16] Fix formating of variables with type: size_t
ACK
Subject: [PATCH 09/16] Adding new header for printf formating macros
I wonder if it makes sense to simply add the new header to util.h instead
of adding it to individual sources. It's only ever going to contain macros,
right? But I agree with having the file separately from util.h it might
be needed in cases where we don't want to include the whole util.h.
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?
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?
Subject: [PATCH 16/16] Fix warning: data argument not used by format
string
Oof, these are bad. Thank you. ACK.