|
Description
Peter McCurdy
2008-08-25 01:33:31 UTC
Created attachment 18494 [details] [review] Store all time values as signed longs The signed long version of the patch. Created attachment 18495 [details] [review] Store all time values as unsigned longs The ying to the previous patch's yang, this patch stores all time values as unsigned longs. I would lean toward the signed version; because it's simpler. It probably conflicts with one of the patches in the "fix timeouts" patch series from Scott James Remnant, where he cleaned up the overflow possibilities there, so could be worth reviewing along with that. It should be a separate bug for any real discussion, but fwiw on the signed/unsigned char the desired fix is to never use "unsigned char" imo. This will be a huge patch unfortunately. (In reply to comment #4) > It should be a separate bug for any real discussion, but fwiw on the > signed/unsigned char That's Bug #15522 now. If we apply (something similar to) one of these, we should enable -Wsign-compare so this doesn't come back. Created attachment 113960 [details] [review] Make all time values signed longs instead of a mix of signed and unsigned, to avoid compiler complaints (rebased) There are already some places like dbus-monitor and dbus-send where signed time values are used, so I would say we should use this variant. Created attachment 113966 [details] [review] Fix of remaining -Wsign-compare issues (In reply to Simon McVittie from comment #6) > If we apply (something similar to) one of these, we should enable > -Wsign-compare so this doesn't come back. This patch fixes the remaining -Wsign-compare issues. Comment on attachment 113960 [details] [review] Make all time values signed longs instead of a mix of signed and unsigned, to avoid compiler complaints (rebased) Review of attachment 113960 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 113960 [details] [review] Make all time values signed longs instead of a mix of signed and unsigned, to avoid compiler complaints (rebased) committed to master Created attachment 113983 [details] [review] Enable -Wsign-compare for cmake builds Created attachment 113984 [details] [review] [5/5] Enable -Wsign-compare for cmake builds with cxx related variable name fix Comment on attachment 113966 [details] [review] Fix of remaining -Wsign-compare issues Review of attachment 113966 [details] [review]: ----------------------------------------------------------------- ::: bus/signals.c @@ +848,5 @@ > > if (end != length) > { > + int len1 = strlen ("path"); > + if ((end + len1) == length && Slightly ugly, but OK ::: dbus/dbus-message.c @@ +602,4 @@ > close_unix_fds(int *fds, unsigned *n_fds) > { > DBusError e; > + unsigned int i; Fine ::: dbus/dbus-sysdeps-unix.c @@ +1738,5 @@ > goto out; > } > > + int len1 = strlen (_dbus_string_get_const_data (&buf)); > + if (len1 != len) This is not OK: len could be uninitialized at this point, if the initial getsockopt() succeeded (which it often will). You probably haven't tested this function, because it needs a Linux system with a LSM - SELinux, AppArmor or Smack - enabled. I would prefer to not do any of your changes that touch this function - I think making these lengths signed is the wrong direction to be going. I'll work out an alternative. ::: tools/dbus-print-message.c @@ +68,4 @@ > > static void > print_hex (const unsigned char *bytes, > + unsigned int len, OK ::: tools/dbus-spam.c @@ +159,2 @@ > int received = 0; > + unsigned int received_before_this_conn = 0; OK Comment on attachment 113966 [details] [review] Fix of remaining -Wsign-compare issues committed to master without the dbus/dbus-sysdeps-unix.c patch mentioned in comment 13 Comment on attachment 18495 [details] [review] Store all time values as unsigned longs rejected, the signed variant has been used Created attachment 113988 [details] [review] -Wsign-compare fixes From: Ralf Habacker <ralf.habacker@freenet.de> [smcv: remove the part that touches add_linux_security_label_to_credentials, which is more subtle] Bug: https://bugs.freedesktop.org/show_bug.cgi?id=17289 Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> (In reply to Simon McVittie from comment #16) > -Wsign-compare fixes > > From: Ralf Habacker <ralf.habacker@freenet.de> > > [smcv: remove the part that touches > add_linux_security_label_to_credentials, which is more subtle] > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=17289 > Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> (never mind, you committed something equivalent) Created attachment 113989 [details] [review] [1/5] Use new _dbus_string_get_ulength() to avoid another -Wsign-compare DBusString's length is signed for historical reasons: the right type would have been size_t or some other unsigned type. We have a *lot* of callers of _dbus_string_get_length(), so it is not really desirable to do a flag-day switch; but we know that the length is always in the range [0, INT_MAX] that is common to int and unsigned int, so we can safely add an unsigned accessor. Created attachment 113990 [details] [review] [2/5] _dbus_listen_systemd_sockets: fds are signed ints (-Wsign-compare) Created attachment 113991 [details] [review] [3/5] fd-passing test: numbers of things are unsigned (-Wsign-compare) --- Presumably you didn't compile the previous one because the CMake build doesn't know about optional functionality like libsystemd, or this one because the CMake build doesn't know about this test. Created attachment 113992 [details] [review] [4/5] Autotools: enable -Wsign-compare and optionally -Werror=sign-compare Comment on attachment 113984 [details] [review] [5/5] Enable -Wsign-compare for cmake builds ... and this one is 5/5 in my patch series. Comment on attachment 113989 [details] [review] [1/5] Use new _dbus_string_get_ulength() to avoid another -Wsign-compare Review of attachment 113989 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-string.h @@ +161,5 @@ > + * that this is the case) so we know that this cast does not change the > + * value. > + */ > +static inline unsigned int > +_dbus_string_get_ulength (const DBusString *str) we already have something like dbus_bool_t _dbus_string_append_int() dbus_bool_t _dbus_string_append_uint() dbus_bool_t _dbus_string_append_byte() better to name it dbus_string_get_length_uint() ::: dbus/dbus-sysdeps-unix.c @@ +1689,4 @@ > _dbus_verbose ("getsockopt failed with %s, len now %lu\n", > _dbus_strerror (e), (unsigned long) len); > > + if (e != ERANGE || len <= _dbus_string_get_ulength (&buf)) _dbus_string_get_length_uint @@ +1714,4 @@ > goto out; > } > > + if (len > _dbus_string_get_ulength (&buf)) dito @@ +1718,3 @@ > { > + _dbus_verbose ("%lu > %u", (unsigned long) len, > + _dbus_string_get_ulength (&buf)); dito (In reply to Ralf Habacker from comment #23) > we already have something like > dbus_bool_t _dbus_string_append_int() > dbus_bool_t _dbus_string_append_uint() > dbus_bool_t _dbus_string_append_byte() That isn't really the same thing: those suffixes are talking about the data being appended, not about the internal representation of the string. But, OK, if you prefer _uint that works. Created attachment 113993 [details] [review] [6] signal_handler: avoid signed/unsigned mismatch (-Wsign-compare) We're ignoring the result of this write() to stderr anyway, because if it failed... what would we do? Write to stderr? That wouldn't work any better the second time :-) --- gcc doesn't complain about this one, but clang does. We know the ssize_t cast is OK here, because the strlen() has a constant result (and in fact I think gcc will optimize it into a compile-time constant). Created attachment 113994 [details] [review] [1/6 v2] Use new _dbus_string_get_length_uint() to avoid another -Wsign-compare Comment on attachment 113994 [details] [review] [1/6 v2] Use new _dbus_string_get_length_uint() to avoid another -Wsign-compare Review of attachment 113994 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 113990 [details] [review] [2/5] _dbus_listen_systemd_sockets: fds are signed ints (-Wsign-compare) Review of attachment 113990 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 113993 [details] [review] [6] signal_handler: avoid signed/unsigned mismatch (-Wsign-compare) Review of attachment 113993 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 113992 [details] [review] [4/5] Autotools: enable -Wsign-compare and optionally -Werror=sign-compare Review of attachment 113992 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 113994 [details] [review] [1/6 v2] Use new _dbus_string_get_length_uint() to avoid another -Wsign-compare Review of attachment 113994 [details] [review]: ----------------------------------------------------------------- With cmake I still got sign-compare related warning because of a bug in cmake build system not defining HAVE_SOCKLEN_T although socklen_t is present. In that case socklen_t is defined as signed int. #ifndef HAVE_SOCKLEN_T #define socklen_t int #endif Created attachment 113996 [details] [review] Fix broken cmake HAVE_SOCKLEN_T type finding check Comment on attachment 113996 [details] [review] Fix broken cmake HAVE_SOCKLEN_T type finding check Review of attachment 113996 [details] [review]: ----------------------------------------------------------------- Good catch (types aren't symbols so I'm not surprised it failed), but the new check doesn't look right either. ::: cmake/ConfigureChecks.cmake @@ +56,5 @@ > check_type_size("long" SIZEOF_LONG) > check_type_size("long long" SIZEOF_LONG_LONG) > check_type_size("__int64" SIZEOF___INT64) > +set(CMAKE_EXTRA_INCLUDE_FILES "sys/socket.h") > +check_type_size(socklen_t HAVE_SOCKLEN_T) # dbus-sysdeps-unix.c This seems like the wrong check - if we have socklen_t it'll #define HAVE_SOCKLEN_T 4 (or whatever) and #define HAVE_HAVE_SOCKLEN_T to some undocumented value, if we don't have socklen_t it isn't clear to me from the documentation what would happen. Given that we only really care about Linux and Windows for CMake, I'm very tempted to do this in config.h.cmake instead: #ifdef DBUS_UNIX /* If your Unix doesn't have socklen_t, use Autotools instead. */ # define HAVE_SOCKLEN_T 1 #endif Alternatively, you could do something like this: set(CMAKE_EXTRA_INCLUDE_FILES "sys/socket.h") check_type_size("socklen_t" SIZEOF_SOCKLEN_T) set(CMAKE_EXTRA_INCLUDE_FILES) and then in config.h.cmake: #ifdef HAVE_SIZEOF_SOCKLEN_T # define HAVE_SOCKLEN_T 1 #endif (In reply to Ralf Habacker from comment #31) > In that case socklen_t is defined as signed int. Functions that take a socklen_t argument on POSIX systems historically took an int, so this is sort of right... although in practice I don't think D-Bus (or anything else for that matter) is going to work on a non-Windows, non-POSIX platform. POSIX says socklen_t is unsigned, and in practice it's always unsigned int afaics. I don't consider it to be a problem if people on non-POSIX Unix (if such a thing even exists...) get compiler warnings for a signed socklen_t. It's worth fixing the CMake check, but the fix could be pretty simple - e.g. assuming that anyone compiling with CMake on Unix does have socklen_t. (In reply to Simon McVittie from comment #20) > [3/5] fd-passing test: numbers of things are unsigned (-Wsign-compare) You missed this one? (In reply to Simon McVittie from comment #33) > Comment on attachment 113996 [details] [review] [review] > Fix broken cmake HAVE_SOCKLEN_T type finding check > > Review of attachment 113996 [details] [review] [review]: > ----------------------------------------------------------------- > > Good catch (types aren't symbols so I'm not surprised it failed), but the > new check doesn't look right either. > > ::: cmake/ConfigureChecks.cmake > @@ +56,5 @@ > > check_type_size("long" SIZEOF_LONG) > > check_type_size("long long" SIZEOF_LONG_LONG) > > check_type_size("__int64" SIZEOF___INT64) > > +set(CMAKE_EXTRA_INCLUDE_FILES "sys/socket.h") > > +check_type_size(socklen_t HAVE_SOCKLEN_T) # dbus-sysdeps-unix.c > > This seems like the wrong check - if we have socklen_t it'll #define > HAVE_SOCKLEN_T 4 (or whatever) and #define HAVE_HAVE_SOCKLEN_T to some > undocumented value, if we don't have socklen_t it isn't clear to me from the > documentation what would happen. According to http://www.cmake.org/cmake/help/v3.0/module/CheckTypeSize.html it says: Check if the type exists and determine its size. On return, “HAVE_${VARIABLE}” holds the existence of the type, and “${VARIABLE}” holds one of the following: <size> = type has non-zero size <size> "0" = type has arch-dependent size (see below) "" = type does not exist So I changed this check to set(CMAKE_EXTRA_INCLUDE_FILES "sys/socket.h") check_type_size("socklen_t" SOCKLEN_T) # dbus-sysdeps-unix.c set(CMAKE_EXTRA_INCLUDE_FILES) message("---------- ${HAVE_SOCKLEN_T} ${SOCKLEN_T}") and got -- Check size of socklen_t -- Check size of socklen_t - done ---------- TRUE 4 I did only check if HAVE_SOCKLEN_T is present or not, which worked. I will update the patch. Created attachment 114001 [details] [review] Fix broken cmake HAVE_SOCKLEN_T type finding check (update) Created attachment 114002 [details] [review] Add test-fdpass to cmake build system. Comment on attachment 113991 [details] [review] [3/5] fd-passing test: numbers of things are unsigned (-Wsign-compare) Review of attachment 113991 [details] [review]: ----------------------------------------------------------------- looks good. (In reply to Ralf Habacker from comment #36) > -- Check size of socklen_t > -- Check size of socklen_t - done > ---------- TRUE 4 > > I did only check if HAVE_SOCKLEN_T is present or not, which worked. I will > update the patch. What happens on a platform where socklen_t does not exist, e.g. Windows? Does the HAVE_SOCKLEN_T cpp macro remain undefined in config.h as desired? Comment on attachment 114002 [details] [review] Add test-fdpass to cmake build system. Review of attachment 114002 [details] [review]: ----------------------------------------------------------------- fine Comment on attachment 113984 [details] [review] [5/5] Enable -Wsign-compare for cmake builds applied Comment on attachment 113990 [details] [review] [2/5] _dbus_listen_systemd_sockets: fds are signed ints (-Wsign-compare) applied Comment on attachment 113991 [details] [review] [3/5] fd-passing test: numbers of things are unsigned (-Wsign-compare) applied Comment on attachment 113992 [details] [review] [4/5] Autotools: enable -Wsign-compare and optionally -Werror=sign-compare applied Comment on attachment 113993 [details] [review] [6] signal_handler: avoid signed/unsigned mismatch (-Wsign-compare) applied Comment on attachment 113994 [details] [review] [1/6 v2] Use new _dbus_string_get_length_uint() to avoid another -Wsign-compare applied Comment on attachment 114002 [details] [review] Add test-fdpass to cmake build system. applied (In reply to Simon McVittie from comment #40) > (In reply to Ralf Habacker from comment #36) > > -- Check size of socklen_t > > -- Check size of socklen_t - done > > ---------- TRUE 4 > > > > I did only check if HAVE_SOCKLEN_T is present or not, which worked. I will > > update the patch. > > What happens on a platform where socklen_t does not exist, e.g. Windows? > Does the HAVE_SOCKLEN_T cpp macro remain undefined in config.h as desired? If we still have !defined(HAVE_SOCKLEN_T) on platforms without socklen_t (please check), then the new version of this patch is OK to apply too. (In reply to Simon McVittie from comment #40) > (In reply to Ralf Habacker from comment #36) > > -- Check size of socklen_t > > -- Check size of socklen_t - done > > ---------- TRUE 4 > > > > I did only check if HAVE_SOCKLEN_T is present or not, which worked. I will > > update the patch. > > What happens on a platform where socklen_t does not exist, e.g. Windows? > Does the HAVE_SOCKLEN_T cpp macro remain undefined in config.h as desired? yes, see http://www.cmake.org/cmake/help/v3.0/command/configure_file.html?highlight=cmakedefine Input file lines of the form “#cmakedefine VAR ...” will be replaced with either “#define VAR ...” or /* #undef VAR */ depending on whether VAR is set in CMake to any value not considered a false constant (In reply to Simon McVittie from comment #22) > Comment on attachment 113984 [details] [review] [review] > [5/5] Enable -Wsign-compare for cmake builds > > ... and this one is 5/5 in my patch series. With this patch I get the following warnings with mingw: /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function '_dbus_socket_is_invalid': /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:649:15: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] return fd == INVALID_SOCKET ? TRUE : FALSE; ^ /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function '_dbus_connect_tcp_socket_with_nonce': /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:1557:58: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if ((fd = socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET) ^ /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function '_dbus_listen_tcp_socket': /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:1710:58: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] if ((fd = socket (tmp->ai_family, SOCK_STREAM, 0)) == INVALID_SOCKET) ^ This is because fd is an int and SOCKET is defined as unsigned int on windows. (In reply to Simon McVittie from comment #49) > (In reply to Simon McVittie from comment #40) > > (In reply to Ralf Habacker from comment #36) > > > -- Check size of socklen_t > > > -- Check size of socklen_t - done > > > ---------- TRUE 4 > > > > > > I did only check if HAVE_SOCKLEN_T is present or not, which worked. I will > > > update the patch. > > > > What happens on a platform where socklen_t does not exist, e.g. Windows? > > Does the HAVE_SOCKLEN_T cpp macro remain undefined in config.h as desired? > > If we still have !defined(HAVE_SOCKLEN_T) on platforms without socklen_t > (please check), then the new version of this patch is OK to apply too. checked with cross compiled mingw ... config.h /* Define to 1 if you have socklen_t */ /* #undef HAVE_SOCKLEN_T */ One remaining issue is on unix platforms not having socklen_t defined. The fallback mentioned in comment 31 defines socklen_t as signed int, which will conflicts with attachment 113994 [details] [review] (In reply to Ralf Habacker from comment #51) > With this patch I get the following warnings with mingw: > > /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c: In function > '_dbus_socket_is_invalid': > /home/ralf.habacker/src/dbus-master/dbus/dbus-sysdeps-win.c:649:15: warning: > comparison between signed and unsigned integer expressions [-Wsign-compare] > return fd == INVALID_SOCKET ? TRUE : FALSE; That's a larger mess which I think we should track separately. I've opened Bug #89444. (In reply to Ralf Habacker from comment #52) > One remaining issue is on unix platforms not having socklen_t defined. The > fallback mentioned in comment 31 defines socklen_t as signed int, which will > conflicts with attachment 113994 [details] [review] I'm not convinced those platforms actually exist. If they do, they are not POSIX-compliant, and I'm fine with them getting compiler warnings for now; we can deal with those if someone reports this actually happening in the real world. I think we're done with this now, except for Bug #89444? Fixed in git for 1.9.16. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.