Re: Verifying printf() formatting of types
On Fri, Mar 22, 2019 at 7:23 AM Stefan Sperling wrote: > > INT64_C() returns a 'LL'-suffixed constant on OpenBSD, and int64_t > is indeed a 'long long': > *That's* what I've been missing, thanks! BSD is wrong. It should have been using the "fastest" shortest-int that represented 64-bits, but I'm sure this is just legacy going back to 32IL/64Q architectures. Irrespective of that... > Type checking in printf format strings is stricter than APR's configure > script logic. > s/is/was/ ... APR_CHECK_TYPES_FMT_COMPATIBLE corrects for printf patterns and types. It would still not pick up the INT64_C exception because we didn't test our constants, only the type apr_int64_t (aka long). Relying entirely on inttypes.h is a 100% solve (modulo a catch for missing format macros) that should work around all platform quirks.
Re: Verifying printf() formatting of types
On Fri, Mar 22, 2019 at 11:55:27AM +, Yann Ylavic wrote: > On Fri, Mar 22, 2019 at 9:45 AM Yann Ylavic wrote: > > Both apr_time_t and apr_interval_time_t are (apr_)int64_t, which is > > probably format "%lld" on OpenBSD, so we'd need to determine > > APR_INT64_T (and all of our numeric _FMT) with > > APR_CHECK_TYPES_FMT_COMPATIBLE too. > > So what about r1856042? Stefan, works for you (this is trunk only for now)? Yes, thank you! Looks much better. Here's diff between generated apr.h from before and after r1856042 on OpenBSD/amd64. --- /tmp/apr.h-before Fri Mar 22 13:25:53 2019 +++ include/apr.h Fri Mar 22 13:28:04 2019 @@ -96,6 +96,7 @@ #define APR_HAVE_STDLIB_H1 #define APR_HAVE_STRING_H1 #define APR_HAVE_STRINGS_H 1 +#define APR_HAVE_INTTYPES_H 1 #define APR_HAVE_SYS_IOCTL_H 1 #define APR_HAVE_SYS_SENDFILE_H 0 #define APR_HAVE_SYS_SIGNAL_H1 @@ -196,6 +197,10 @@ #include #endif +#if APR_HAVE_INTTYPES_H +#include +#endif + #if APR_HAVE_SYS_WAIT_H #include #endif @@ -376,8 +381,8 @@ #define UINT64_C(v) (v ## ULL) #endif #else - typedef longapr_int64_t; - typedef unsigned long apr_uint64_t; + typedef int64_t apr_int64_t; + typedef uint64_t apr_uint64_t; #endif typedef size_t apr_size_t; @@ -586,19 +591,19 @@ #define APR_SIZE_T_FMT "lu" /* And APR_OFF_T_FMT */ -#define APR_OFF_T_FMT "ld" +#define APR_OFF_T_FMT "lld" /* And APR_PID_T_FMT */ #define APR_PID_T_FMT "d" /* And APR_INT64_T_FMT */ -#define APR_INT64_T_FMT "ld" +#define APR_INT64_T_FMT PRId64 /* And APR_UINT64_T_FMT */ -#define APR_UINT64_T_FMT "lu" +#define APR_UINT64_T_FMT PRIu64 /* And APR_UINT64_T_HEX_FMT */ -#define APR_UINT64_T_HEX_FMT "lx" +#define APR_UINT64_T_HEX_FMT PRIx64 /* * Ensure we work with universal binaries on Darwin
Re: Verifying printf() formatting of types
On Fri, Mar 22, 2019 at 09:45:38AM +, Yann Ylavic wrote: > Both apr_time_t and apr_interval_time_t are (apr_)int64_t, which is > probably format "%lld" on OpenBSD, so we'd need to determine > APR_INT64_T (and all of our numeric _FMT) with > APR_CHECK_TYPES_FMT_COMPATIBLE too. Yes, I agree. From my point of view, the problem looks as follows: On OpenBSD/amd64, APR's configure script sees that 'sizeof(long)' and 'sizeof(long long)' are equivalent. It then makes an implicit assumption that all 64 bit types and related constants in C header files are dealt with in 'long' rather than in 'long long'. But this assumption is wrong. INT64_C() returns a 'LL'-suffixed constant on OpenBSD, and int64_t is indeed a 'long long': /usr/include/machine/_types.h:typedef long long __int64_t; /usr/include/sys/stdint.h:typedef __int64_t int64_t; Type checking in printf format strings is stricter than APR's configure script logic. The compiler will complain even if parameters are of the same size but not the same type (e.g. "%ld" format for a long long variable). This makes sense because the format string will end up being used on a variety of platforms with varying sizes for integer types, and printf cannot know that APR is trying to provide a matching format directive via custom macros. A non-APR C program will just hard-code format strings and as long as the format string and the type are an exact match, the program will be fine. So that's what the C compiler checks for. So the answer seems to be: APR needs to improve its configure script logic to match the strictness of format string checks as implemented by gcc and clang when mapping integer types defined by the underlying system to APR's own integer types. Else these warnings will show up one way or another. It cannot just assume that 'long' is preferred over 'long long' or vice versa.
Re: Verifying printf() formatting of types
On Fri, Mar 22, 2019 at 9:45 AM Yann Ylavic wrote: > > On Fri, Mar 22, 2019 at 9:24 AM Ruediger Pluem wrote: > > > > On 03/20/2019 11:00 AM, Stefan Sperling wrote: > > > On Tue, Mar 19, 2019 at 07:30:09PM -0500, William A Rowe Jr wrote: > > >> According to my observations, apr_time_t should match the APR_TIME_T_FMT > > >> token in every case. Please inspect that line of httpd code to see how > > >> some > > >> non-apr_time_t value was passed in APR_TIME_T_FMT formatting. > > > > > > Indeed, this value is not a time_t, it's an apr_int64_t, i.e. long. > > > > > > The problematic format string is in this bit code from proxy_util.c > > > starting at line 3176: > > > > > > ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959) > > > "ap_proxy_connect_backend disabling worker for (%s) > > > for %" > > > APR_TIME_T_FMT "s", > > > worker->s->hostname_ex, > > > apr_time_sec(worker->s->retry)); > > > > > > This assumes apr_time_sec returns apr_time_t, but in fact apr_time_sec is > > > a macro. So the expression returns the type of the variable passed in, > > > which in this case is apr_interval_time_t. > > > > Possibly stupid idea, but what if the macro does a cast to apr_time_t? > > Would that solve the issue? > > Both apr_time_t and apr_interval_time_t are (apr_)int64_t, which is > probably format "%lld" on OpenBSD, so we'd need to determine > APR_INT64_T (and all of our numeric _FMT) with > APR_CHECK_TYPES_FMT_COMPATIBLE too. So what about r1856042? Stefan, works for you (this is trunk only for now)?
Re: Verifying printf() formatting of types
On Fri, Mar 22, 2019 at 9:24 AM Ruediger Pluem wrote: > > On 03/20/2019 11:00 AM, Stefan Sperling wrote: > > On Tue, Mar 19, 2019 at 07:30:09PM -0500, William A Rowe Jr wrote: > >> According to my observations, apr_time_t should match the APR_TIME_T_FMT > >> token in every case. Please inspect that line of httpd code to see how some > >> non-apr_time_t value was passed in APR_TIME_T_FMT formatting. > > > > Indeed, this value is not a time_t, it's an apr_int64_t, i.e. long. > > > > The problematic format string is in this bit code from proxy_util.c > > starting at line 3176: > > > > ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959) > > "ap_proxy_connect_backend disabling worker for (%s) for > > %" > > APR_TIME_T_FMT "s", > > worker->s->hostname_ex, apr_time_sec(worker->s->retry)); > > > > This assumes apr_time_sec returns apr_time_t, but in fact apr_time_sec is > > a macro. So the expression returns the type of the variable passed in, > > which in this case is apr_interval_time_t. > > Possibly stupid idea, but what if the macro does a cast to apr_time_t? Would > that solve the issue? Both apr_time_t and apr_interval_time_t are (apr_)int64_t, which is probably format "%lld" on OpenBSD, so we'd need to determine APR_INT64_T (and all of our numeric _FMT) with APR_CHECK_TYPES_FMT_COMPATIBLE too. Regards, Yann.
Re: Verifying printf() formatting of types
On 03/20/2019 11:00 AM, Stefan Sperling wrote: > On Tue, Mar 19, 2019 at 07:30:09PM -0500, William A Rowe Jr wrote: >> According to my observations, apr_time_t should match the APR_TIME_T_FMT >> token in every case. Please inspect that line of httpd code to see how some >> non-apr_time_t value was passed in APR_TIME_T_FMT formatting. > > Indeed, this value is not a time_t, it's an apr_int64_t, i.e. long. > > The problematic format string is in this bit code from proxy_util.c > starting at line 3176: > > ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959) > > "ap_proxy_connect_backend disabling worker for (%s) for > %" > APR_TIME_T_FMT "s", > worker->s->hostname_ex, apr_time_sec(worker->s->retry)); > > This assumes apr_time_sec returns apr_time_t, but in fact apr_time_sec is > a macro. So the expression returns the type of the variable passed in, > which in this case is apr_interval_time_t. Possibly stupid idea, but what if the macro does a cast to apr_time_t? Would that solve the issue? Regards Rüdiger
Re: Verifying printf() formatting of types
On Tue, Mar 19, 2019 at 07:30:09PM -0500, William A Rowe Jr wrote: > According to my observations, apr_time_t should match the APR_TIME_T_FMT > token in every case. Please inspect that line of httpd code to see how some > non-apr_time_t value was passed in APR_TIME_T_FMT formatting. Indeed, this value is not a time_t, it's an apr_int64_t, i.e. long. The problematic format string is in this bit code from proxy_util.c starting at line 3176: ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959) "ap_proxy_connect_backend disabling worker for (%s) for %" APR_TIME_T_FMT "s", worker->s->hostname_ex, apr_time_sec(worker->s->retry)); This assumes apr_time_sec returns apr_time_t, but in fact apr_time_sec is a macro. So the expression returns the type of the variable passed in, which in this case is apr_interval_time_t. from mod_proxy.h: apr_interval_time_t retry; /* retry interval */ and from apr_time.h: typedef apr_int64_t apr_interval_time_t; So we need this patch: --- modules/proxy/proxy_util.c (revision 1855812) +++ modules/proxy/proxy_util.c (working copy) @@ -3175,7 +3175,7 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const worker->s->status |= PROXY_WORKER_IN_ERROR; ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(00959) "ap_proxy_connect_backend disabling worker for (%s) for %" -APR_TIME_T_FMT "s", +APR_INT64_T_FMT "s", worker->s->hostname_ex, apr_time_sec(worker->s->retry)); } } But even with this patch, the error remains: [[[ cc1: warnings being treated as errors proxy_util.c: In function 'ap_proxy_connect_backend': proxy_util.c:3179: warning: format '%ld' expects type 'long int', but argument 9 has type 'long long int' ]]] This was confusing me at first: apr_int64_t is a long, so why does the compiler see a 'long long' type? I then realized that httpd's configure script was picking up the ancient GPLv2 version of gcc which is still installed even though it's rarely used. I forced my httpd turnk build to use clang and got a more informative warning: [[[ proxy_util.c:3179:45: warning: format specifies type 'long' but the argument has type 'long long' [-Wformat] worker->s->hostname_ex, apr_time_sec(worker->s->retry)); ^~~ /tmp/apr/include/apr-2/apr_time.h:63:28: note: expanded from macro 'apr_time_sec' #define apr_time_sec(time) ((time) / APR_USEC_PER_SEC) ]]] Turns out the remaining problem lies in the definition of APR_USEC_PER_SEC: /** number of microseconds per second */ #define APR_USEC_PER_SEC APR_TIME_C(100) Following the chain of definitions, we arrive at a constant suffixed with LL: /* Mechanisms to properly type numeric literals */ #define APR_INT64_C(val) INT64_C(val) /usr/include/sys/stdint.h:#define INT64_C(_c) __CONCAT(_c, LL) /usr/include/sys/stdint.h:#define UINT64_C(_c)__CONCAT(_c, ULL) Yet APR blindly assumes that INT64_C results in constants suffixed with just one L (long)! A similar problem exists for time_t, by the way. OpenBSD defines time_t as 'long long', but apr_time_t is defined as 'long' in a 64 bit environment. This means that apr_time_t won't have the same type as time_t on 64 bit OpenBSD architectures. On 32 bit platforms, apr_time_t and time_t are both defined as 'long long'. OpenBSD's convention is to use '%lld' for printing time_t, regardless of the CPU platform -- all platforms use 64-bit time_t since OpenBSD 5.5: https://www.openbsd.org/lyrics.html#55
Re: Verifying printf() formatting of types
On Tue, Mar 19, 2019 at 6:35 PM Stefan Sperling wrote: > On Tue, Mar 19, 2019 at 02:09:51PM -0500, William A Rowe Jr wrote: > > I find no errors in our APR_TIME -> APR_INT64 determination, and > > made no such changes. > > > > But I note httpd 2.4.x commit r1824504 by Yann made a correction in > > this sphere. Are you testing the current code or an old 2.4 release? > > If it is the release, please retest using 2.4.x branch? > > I tried httpd trunk as of r1855812. > > I doubt this time_t format problem is new. I likely never noticed > it before because it is getting drowned by all the off_t errors. > According to my observations, apr_time_t should match the APR_TIME_T_FMT token in every case. Please inspect that line of httpd code to see how some non-apr_time_t value was passed in APR_TIME_T_FMT formatting. > Your diff is a great step forward in any case. > Glad this solve is working.
Re: Verifying printf() formatting of types
On Tue, Mar 19, 2019 at 02:09:51PM -0500, William A Rowe Jr wrote: > I find no errors in our APR_TIME -> APR_INT64 determination, and > made no such changes. > > But I note httpd 2.4.x commit r1824504 by Yann made a correction in > this sphere. Are you testing the current code or an old 2.4 release? > If it is the release, please retest using 2.4.x branch? I tried httpd trunk as of r1855812. I doubt this time_t format problem is new. I likely never noticed it before because it is getting drowned by all the off_t errors. Your diff is a great step forward in any case.
Re: Verifying printf() formatting of types
On Tue, Mar 19, 2019 at 5:18 AM Stefan Sperling wrote: > I don't believe your diff attempts to address this, but I am stilling > seeing a problem with APR_TIME_T_FMT when compiling httpd: > > proxy_util.c:3179: warning: format '%ld' expects type 'long int', but > argument 9 has type 'long long int' > I find no errors in our APR_TIME -> APR_INT64 determination, and made no such changes. But I note httpd 2.4.x commit r1824504 by Yann made a correction in this sphere. Are you testing the current code or an old 2.4 release? If it is the release, please retest using 2.4.x branch? > Results for off_t look good. All platforms listed below use clang 6.0.0, > except sparc64 which still uses ancient GPLv2 GCC 4.2.1 20070719. > Thank you for the thorough review!
Re: Verifying printf() formatting of types
On Mon, Mar 18, 2019 at 05:09:40PM -0500, William A Rowe Jr wrote: > Hi folks, > > please test the attached, it works for me on Fedora, but need to verify at > least BSD, Solaris, etc. mingw remains special-case so it shouldn't be > affected. This should force off_t_fmt to "ldd" in the cases Stefan observed. > > Notably, it might not do the right thing with respect to strictness until > we cover a particular c compiler in APR_TRY_COMPILE_NO_WARNING(). The > mismatched int type assignments should crack under most any compiler, but > mismatched printf formatting might not. APR_CHECK_TYPES_FMT_COMPATIBLE() > uses that logic to perform a cross-platform compile torture test against > our [s]size_t/off_t types, in place of the very peculiar > APR_CHECK_TYPES_COMPATIBLE() which didn't appear to be portable beyond gcc > at all, and was much too tolerant of language warnings. > > I personally think it should be a little more robust about checking that > strtoll/_strtoll/_strtoq is available and appropriate and drop more of the > apr_int64_t assumptions around off_t handling, but that was going a little > too far off in the weeds. > > TIA for your reviews and feedback, > > Bill I don't believe your diff attempts to address this, but I am stilling seeing a problem with APR_TIME_T_FMT when compiling httpd: proxy_util.c:3179: warning: format '%ld' expects type 'long int', but argument 9 has type 'long long int' Results for off_t look good. All platforms listed below use clang 6.0.0, except sparc64 which still uses ancient GPLv2 GCC 4.2.1 20070719. OpenBSD/amd64: $ grep ^ac_cv_sizeof config.log ac_cv_sizeof_char=1 ac_cv_sizeof_ino_t=8 ac_cv_sizeof_int=4 ac_cv_sizeof_long=8 ac_cv_sizeof_long_long=8 ac_cv_sizeof_off_t=8 ac_cv_sizeof_pid_t=4 ac_cv_sizeof_short=2 ac_cv_sizeof_size_t=8 ac_cv_sizeof_ssize_t=8 ac_cv_sizeof_struct_iovec=16 ac_cv_sizeof_voidp=8 before: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "ld"' off_t_fmt='#define APR_OFF_T_FMT "ld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "lu"' after: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "ld"' off_t_fmt='#define APR_OFF_T_FMT "lld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "lu"' OpenBSD/i386: $ grep ^ac_cv_sizeof config.log ac_cv_sizeof_char=1 ac_cv_sizeof_ino_t=8 ac_cv_sizeof_int=4 ac_cv_sizeof_long=4 ac_cv_sizeof_long_long=8 ac_cv_sizeof_off_t=8 ac_cv_sizeof_pid_t=4 ac_cv_sizeof_short=2 ac_cv_sizeof_size_t=4 ac_cv_sizeof_ssize_t=4 ac_cv_sizeof_struct_iovec=8 ac_cv_sizeof_voidp=4 before: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "lld"' off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "llu"' after: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "lld" off_t_fmt='#define APR_OFF_T_FMT "lld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "llu"' OpenBSD/sparc64: $ grep ^ac_cv_sizeof config.log ac_cv_sizeof_char=1 ac_cv_sizeof_ino_t=8 ac_cv_sizeof_int=4 ac_cv_sizeof_long=8 ac_cv_sizeof_long_long=8 ac_cv_sizeof_off_t=8 ac_cv_sizeof_pid_t=4 ac_cv_sizeof_short=2 ac_cv_sizeof_size_t=8 ac_cv_sizeof_ssize_t=8 ac_cv_sizeof_struct_iovec=16 ac_cv_sizeof_voidp=8 before: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "ld"' off_t_fmt='#define APR_OFF_T_FMT "ld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "lu"' after: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "ld"' off_t_fmt='#define APR_OFF_T_FMT "lld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "lu"' OpenBSD/armv7: $ grep ^ac_cv_sizeof config.log ac_cv_sizeof_char=1 ac_cv_sizeof_ino_t=8 ac_cv_sizeof_int=4 ac_cv_sizeof_long=4 ac_cv_sizeof_long_long=8 ac_cv_sizeof_off_t=8 ac_cv_sizeof_pid_t=4 ac_cv_sizeof_short=2 ac_cv_sizeof_size_t=4 ac_cv_sizeof_ssize_t=4 ac_cv_sizeof_struct_iovec=8 ac_cv_sizeof_voidp=4 before: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "lld"' off_t_fmt='#define APR_OFF_T_FMT APR_INT64_T_FMT' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define APR_SIZE_T_FMT "lu"' ssize_t_fmt='#define APR_SSIZE_T_FMT "ld"' uint64_t_fmt='#define APR_UINT64_T_FMT "llu"' after: $ grep T_FMT config.log int64_t_fmt='#define APR_INT64_T_FMT "lld"' off_t_fmt='#define APR_OFF_T_FMT "lld"' pid_t_fmt='#define APR_PID_T_FMT "d"' size_t_fmt='#define
Re: Verifying printf() formatting of types
On Mon, Mar 18, 2019 at 5:09 PM William A Rowe Jr wrote: > > please test the attached, it works for me on Fedora, but need to verify at > least BSD, Solaris, etc. mingw remains special-case so it shouldn't be > affected. This should force off_t_fmt to "ldd" in the cases Stefan observed. > > Notably, it might not do the right thing with respect to strictness until > we cover a particular c compiler in APR_TRY_COMPILE_NO_WARNING(). The > mismatched int type assignments should crack under most any compiler, but > mismatched printf formatting might not. APR_CHECK_TYPES_FMT_COMPATIBLE() > uses that logic to perform a cross-platform compile torture test against > our [s]size_t/off_t types, in place of the very peculiar > APR_CHECK_TYPES_COMPATIBLE() which didn't appear to be portable beyond gcc > at all, and was much too tolerant of language warnings. > > I personally think it should be a little more robust about checking that > strtoll/_strtoll/_strtoq is available and appropriate and drop more of the > apr_int64_t assumptions around off_t handling, but that was going a little > too far off in the weeds. > Not so far off, the logic in the final patch will be; APR_CHECK_TYPES_FMT_COMPATIBLE(ssize_t, long, ld, [ssize_t_fmt="ld"], [ APR_CHECK_TYPES_FMT_COMPATIBLE(ssize_t, int, d, [ssize_t_fmt="d"]) ]) APR_CHECK_TYPES_FMT_COMPATIBLE(size_t, unsigned long, lu, [size_t_fmt="lu"], [ APR_CHECK_TYPES_FMT_COMPATIBLE(size_t, unsigned int, u, [size_t_fmt="u"]) ]) in order to prefer long over int, where both are acceptable mappings, as the code in 1.6.x has done. The preferences in off_t logic should already be correct as proposed.
