|
Description
Ralf Habacker
2015-02-23 18:42:01 UTC
We explicitly suppress some of these (notably -Wpointer-sign), so I don't know why you're still seeing them? Others are known false positives. Others are probably bugs. I'll take a look tomorrow. (In reply to Simon McVittie from comment #1) > We explicitly suppress some of these (notably -Wpointer-sign), so I don't > know why you're still seeing them? I'm using cmake. May be there are different compile options used. > Others are probably bugs. This /home/ralf/src/dbus/cmake/../dbus/dbus-internals.h:205:66: warning: the comparison will always evaluate as 'false' for the address of 'tmp_error' will never be NULL [-Waddress] #define _DBUS_ASSERT_ERROR_IS_SET(error) _dbus_assert ((error) == NULL || dbus_error_is_set ((error))) and this one /home/ralf/src/dbus/cmake/../dbus/dbus-internals.h:206:66: warning: the comparison will always evaluate as 'false' for the address of 'tmp_error' will never be NULL [-Waddress] #define _DBUS_ASSERT_ERROR_IS_CLEAR(error) _dbus_assert ((error) == NULL || !dbus_error_is_set ((error))) are raised using the following code fragment: DBusError error; _DBUS_ASSERT_ERROR_IS_CLEAR(&error) > > I'll take a look tomorrow. Thanks Created attachment 113808 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' for the address of '....' will never be NULL [-Waddress]' Comment on attachment 113808 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' for the address of '....' will never be NULL [-Waddress]' Review of attachment 113808 [details] [review]: ----------------------------------------------------------------- This adds the cost of an extern function call to these assertions. Worse than that, you've made them DBUS_PRIVATE_EXPORT, so the call goes through the PLT and cannot be inlined even with LTO; higher cost again. Possible solution 1: replace _DBUS_ASSERT_ERROR_IS_SET (&e) with _dbus_assert (dbus_error_is_set (&e)) every time it's used: the additional check in _DBUS_ASSERT_ERROR_IS_SET (handling NULL) is pointless for errors on the stack. The same for _DBUS_ASSERT_ERROR_IS_CLEAR. Possible solution 2: put _dbus_assert_error_is_set(), _dbus_assert_error_is_clear in dbus-internals.h as static inline functions (which are often a better answer than macros). In Autotools-land, if necessary inline is automatically a #define for __inline__ or __inline or nothing (via AC_C_INLINE) so "static inline void foo (void) {}" should work everywhere. In CMake, we #define it to __inline on MSVC, so again it should work on all the platforms we care about for CMake (Linux gcc, mingw gcc and MSVC). (In reply to Simon McVittie from comment #4) > Possible solution 1: replace _DBUS_ASSERT_ERROR_IS_SET (&e) with > _dbus_assert (dbus_error_is_set (&e)) every time it's used To be clear, I mean changing _DBUS_ASSERT_ERROR_IS_SET (&e) where e is a DBusError, but not changing _DBUS_ASSERT_ERROR_IS_SET (e) where e is a DBusError * (that might still be NULL). Created attachment 113814 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' (update) (In reply to Simon McVittie from comment #4) > Possible solution 2: put _dbus_assert_error_is_set(), > _dbus_assert_error_is_clear in dbus-internals.h as static inline functions > (which are often a better answer than macros). yes, they are type save :-) see updated patch (In reply to Ralf Habacker from comment #6) > Created attachment 113814 [details] [review] [review] > Fix warning: 'the comparison will always evaluate as 'false' (update) BTW: why not replace _DBUS_ASSERT_ERROR_IS_SET/CLEAR completly with _dbus_assert_error_is_set/clear Comment on attachment 113814 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' (update) Review of attachment 113814 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-internals.h @@ +204,5 @@ > #else > +static inline void _dbus_assert_error_is_set(const DBusError *error) > +{ > + _dbus_assert (error == NULL || dbus_error_is_set (error)); > +} Coding style nitpicking: static inline void /* ^ new line between return type and definition (but not * forward declarations, so you can grep for '^function_name' * and find only the definition) */ _dbus_assert_error_is_set (const DBusError *error) /* ^ space before open parenthesis */ { /* <- 2-space indent */ _dbus_assert (blah blah); } but functionally, this is fine. (In reply to Ralf Habacker from comment #8) > BTW: why not replace _DBUS_ASSERT_ERROR_IS_SET/CLEAR completly with > _dbus_assert_error_is_set/clear No strong reason, but the diff would be rather large, making merge conflicts more likely; there's no strong reason to do it either, so I would say leave it alone. Created attachment 113842 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' (update 2) (In reply to Simon McVittie from comment #10) > (In reply to Ralf Habacker from comment #8) > > BTW: why not replace _DBUS_ASSERT_ERROR_IS_SET/CLEAR completly with > > _dbus_assert_error_is_set/clear > > No strong reason, but the diff would be rather large, making merge conflicts > more likely; there's no strong reason to do it either, so I would say leave > it alone. you are refering to 1.8 branch, which may be merged into master ? If so, this patch should also be applied to 1.8 to keep branches in sync. (In reply to Ralf Habacker from comment #12) > > No strong reason, but the diff would be rather large, making merge conflicts > > more likely; there's no strong reason to do it either, so I would say leave > > it alone. > > you are refering to 1.8 branch, which may be merged into master ? If so, > this patch should also be applied to 1.8 to keep branches in sync. No, this patch should definitely not be applied to 1.8: it is not a minimal targeted fix for a serious functional bug in D-Bus. My primary consideration for changes to stable branches is: would a conservative reviewer like the Debian release team accept this as "obviously a correct, targeted fix for a real bug"? If not, then we should not make the change in a stable branch, because the whole point of the stable-branch releases is that they can be used by conservative stable distributions like Debian, RHEL and Ubuntu LTS. Making "stable" releases that have too much code churn for their intended audience to accept them is not a useful thing for us to be spending time on :-) I'm willing to make large changes in the development branch whenever their benefit is greater than their cost (where "number of conflicts when merging from 1.8" is part of the cost). For instance, when I made it possible for _DBUS_LOCK() to fail on OOM, that was a tree-wide change that probably caused conflicts: but the benefit, being able to make libdbus thread-safe by default, was enough to justify it. In this case, the cost is not particularly large (maybe some conflicts), but the benefit is also not very large, so I'm not convinced it would be worth it. Comment on attachment 113842 [details] [review] Fix warning: 'the comparison will always evaluate as 'false' (update 2) patch has been reviewed in comment 9: committed to master Created attachment 113946 [details]
dedicated pointer-sign, conversion, sign-conversion warning log
(In reply to Simon McVittie from comment #13) > (In reply to Ralf Habacker from comment #12) > > > No strong reason, but the diff would be rather large, making merge conflicts > > > more likely; there's no strong reason to do it either, so I would say leave > > > it alone. > > > > you are refering to 1.8 branch, which may be merged into master ? If so, > > this patch should also be applied to 1.8 to keep branches in sync. > > No, this patch should definitely not be applied to 1.8: it is not a minimal > targeted fix for a serious functional bug in D-Bus. I guess you are refering to the question about the upper-lowercase rename, which I understand, but as far as I can see does dbus have a rule to use uppercase for macros and lower case for funtion name. Following this rule needs to change uppercase to lowercase sometime in the future. The question is when ? > (In reply to Simon McVittie from comment #1)
> > We explicitly suppress some of these (notably -Wpointer-sign), so I don't
> > know why you're still seeing them?
> I'm using cmake. May be there are different compile options used.
Just for interest I compiled dbus on linux with -Wpointer-sign, -Wconversion, -Wsign-conversion warnings enabled and got
grep Wsign warning.log | wc -l
365
grep Wconvers warning.log | wc -l
325
grep Wpointer warning.log | wc -l
105
I'm surprised that a security related software like dbus has so many sign warnings in the code.
How could this be interpreted ? Those warnings are suppressed because it is sure that there are no possible side effects or sign overflows. Could this be guaranteed ?
How could the numbers be interpreted ?
1. There is something designed wrongly with dbus api so people are not able to program sign safe
3. No one cares about programming sign safe (maybe because of suppressing warnings)
(In reply to Ralf Habacker from comment #2) > (In reply to Simon McVittie from comment #1) > > We explicitly suppress some of these (notably -Wpointer-sign), so I don't > > know why you're still seeing them? > > I'm using cmake. May be there are different compile options used. There are. Under Autotools, we explicitly suppress: * -Wmissing-field-initializers (deliberate choice, not a bug) * -Wunused-parameter (deliberate choice, not a bug) * some warnings of the form -Wunused-foo if assertions/tests are disabled (deliberate choice, not a bug) * -Wsign-compare, -Wpointer-sign (Bug #17433, please send any discussion there) * -Wtype-limits (is a bug in principle, but not yet filed) if the compiler supports appropriate -Wno-foo options (in practice that means gcc). The -Wsign-conversion warnings and some of the -Wconversion are similar to Bug #17433, afaics. To get a more manageable set, I would suggest silencing those, temporarily silencing -Wsign-conversion and -Wconversion, and fixing anything that's left, then re-enabling the temporarily silenced warnings gradually. Some of these type mismatches probably have an obvious correct fix, but some of them are probably really subtle. As with Bug #17433, I don't want to "fix" them by scattering casts around without thinking about the right semantics; we should do it right (where the meaning of "right" is probably a case-by-case decision) or not at all. Some of these are false positives, for instance: > warning: conversion to ‘long unsigned int’ from ‘WriteResult’ > may change the sign of the result [-Wsign-conversion] The code is: ifdef DBUS_WIN typedef int WriteResult; #define write(fd, buf, len) _write(fd, buf, len) #else typedef ssize_t WriteResult; #endif ... size_t bytes_written = 0; while (size > bytes_written) { WriteResult this_time = write (fd, p, size - bytes_written); if (this_time < 0) { if (errno == EINTR) continue; else return FALSE; } p += this_time; bytes_written += this_time; // <- line 94 } The implicit conversion of this_time from int or ssize_t to size_t in line 94 can be proved to be safe, because we know this_time is non-negative - so I'm surprised gcc is issuing this warning. Every non-negative int is in-range for size_t on Windows (sizeof(int) is 4 on Windows, and sizeof(size_t) is 4 or 8), and every non-negative ssize_t is in-range for size_t on Unix (sizeof(ssize_t) == sizeof(size_t) by definition). > warning: ‘g_test_trap_fork’ is deprecated (declared at > /usr/include/glib-2.0/glib/gtestutils.h:198): Use 'g_test_trap_subprocess' > instead [-Wdeprecated-declarations] Under Autotools we #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_36 and #define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_38, silencing any deprecations that were added since 2.38. CMake should ideally do the same. (In reply to Ralf Habacker from comment #17) > Just for interest I compiled dbus on linux with -Wpointer-sign, > -Wconversion, -Wsign-conversion warnings enabled See Bug #15522 (sorry, I referred to Bug #17433 earlier, but that was closed as a dup of #8164 which was cloned as Bug #15522 - this is what happens when you have large vague bugs for a broad class of compiler warnings). The majority of -Wpointer-sign warnings are conversions between byte-arrays (for which Havoc semi-consistently used unsigned char *) and UTF-8 strings (for which Havoc semi-consistently used char *). > There is something designed wrongly with dbus api so people are not able to program sign safe There are at least two places known to be designed wrongly: * Bug #17289: are timeouts signed or unsigned? Nobody knows. * Bug #15522: are byte-arrays char * or unsigned char *? Nobody knows (in particular, lots of C/POSIX/Windows APIs use char * but algorithms like UTF-8 really want to deal with unsigned char *). For the timeouts, yes, the behaviour around overflows is going to be subtle. As far as we know, the current implementation works; as soon as we start changing it, there's a reasonable chance of making matters worse. It's a trade-off between the possibility of fixing broken situations, and the posssibility of breaking working situations. https://bugs.freedesktop.org/show_bug.cgi?id=15522#c6 is a concrete suggestion for how someone could address most of the byte-array issues, if anyone cares enough. If we really need casts, then I'm keen on encapsulating them like that, to make it completely obvious that they are valid. I am not keen for this bug to turn into another large, vague "my compiler produces warnings which could indicate subtle errors, but could equally well be false positives" like Bug #8164. If there are a few misc warnings with obvious solutions (e.g. what you get when you silence -Wconversion and -Wsign-conversion), that's a manageable chunk of work to do in one go; but if there are systematic issues like "timeouts might be signed or not", "byte-arrays might have signed char or not", etc., it's better for everyone's sanity if we stick to smaller, more actionable bugs/tasks such as "give timeouts a consistent signedness". Created attachment 113953 [details] [review] Keep cmake defines GLIB_VERSION_... in sync with autotools (In reply to Simon McVittie from comment #18) > (In reply to Ralf Habacker from comment #2) > > (In reply to Simon McVittie from comment #1) > > > We explicitly suppress some of these (notably -Wpointer-sign), so I don't > > > know why you're still seeing them? > > > > I'm using cmake. May be there are different compile options used. > > There are. Under Autotools, we explicitly suppress: > > * -Wmissing-field-initializers (deliberate choice, not a bug) > * -Wunused-parameter (deliberate choice, not a bug) > * some warnings of the form -Wunused-foo if assertions/tests are disabled > (deliberate choice, not a bug) > * -Wsign-compare, -Wpointer-sign (Bug #17433, please send any discussion > there) > * -Wtype-limits (is a bug in principle, but not yet filed) > > if the compiler supports appropriate -Wno-foo options (in practice that > means gcc). > > The -Wsign-conversion warnings and some of the -Wconversion are similar to > Bug #17433, afaics. > > To get a more manageable set, I would suggest silencing those, temporarily > silencing -Wsign-conversion and -Wconversion, and fixing anything that's > left, then re-enabling the temporarily silenced warnings gradually. > > Some of these type mismatches probably have an obvious correct fix, but some > of them are probably really subtle. As with Bug #17433, I don't want to > "fix" them by scattering casts around without thinking about the right > semantics; we should do it right (where the meaning of "right" is probably a > case-by-case decision) or not at all. > > Some of these are false positives, for instance: > > > warning: conversion to ‘long unsigned int’ from ‘WriteResult’ > > may change the sign of the result [-Wsign-conversion] > > The code is: > > ifdef DBUS_WIN > typedef int WriteResult; > #define write(fd, buf, len) _write(fd, buf, len) > #else > typedef ssize_t WriteResult; > #endif > > ... > > size_t bytes_written = 0; > > while (size > bytes_written) > { > WriteResult this_time = write (fd, p, size - bytes_written); > > if (this_time < 0) > { > if (errno == EINTR) > continue; > else > return FALSE; > } > > p += this_time; > bytes_written += this_time; // <- line 94 > } > What about something like this, which compiles without any warnings on linux and windows static inline size_t dbus_write(int fd, const void *buf, unsigned int len) { return (size_t)write(fd, buf, len); } dbus_bool_t tool_write_all (int fd, const void *buf, size_t size) { const char *p = buf; size_t bytes_written = 0; while (size > bytes_written) { size_t this_time = dbus_write (fd, p, size - bytes_written); if (this_time < 0) { if (errno == EINTR) continue; else return FALSE; } p += this_time; bytes_written += this_time; } return TRUE; } Comment on attachment 113953 [details] [review] Keep cmake defines GLIB_VERSION_... in sync with autotools Review of attachment 113953 [details] [review]: ----------------------------------------------------------------- If my assumption below is correct then this looks fine to apply. ::: cmake/config.h.cmake @@ +96,4 @@ > > #cmakedefine DBUS_VA_COPY_AS_ARRAY @DBUS_VA_COPY_AS_ARRAY@ > > +#cmakedefine DBUS_WITH_GLIB 1 Just to check, this is conditional, right? We'll #define DBUS_WITH_GLIB 1 if we have GLib, and leave it undefined if we don't? (In reply to Ralf Habacker from comment #21) > What about something like this, which compiles without any warnings on linux > and windows ... > return (size_t)write(fd, buf, len); This makes a nice example of why adding casts without thinking about the semantics is not the correct solution for warnings like this. Just because the compiler shuts up about it does not mean the code is right. > size_t this_time = dbus_write (fd, p, size - bytes_written); > > if (this_time < 0) ISO C says size_t is an unsigned integer type, so it can't possibly be negative. If the write() fails and returns -1, we will now interpret that as having written 0xFFFFFFFF bytes of data (on 32-bit), which is wrong. This is why POSIX has write() returning ssize_t (not size_t), and why Windows has _write returning int (not unsigned int). The place where it would be valid to add a cast is where you have already proved that it cannot change the value, something like this: WriteResult res = ...; if (res < 0) { /* ... handle error */ } else { /* now known to be non-negative, so this cast is valid */ size_t this_time = (size_t) res; /* ... use this_time as before */ (In reply to Simon McVittie from comment #22) > Comment on attachment 113953 [details] [review] [review] > Keep cmake defines GLIB_VERSION_... in sync with autotools > > Review of attachment 113953 [details] [review] [review]: > ----------------------------------------------------------------- > > If my assumption below is correct then this looks fine to apply. > > ::: cmake/config.h.cmake > @@ +96,4 @@ > > > > #cmakedefine DBUS_VA_COPY_AS_ARRAY @DBUS_VA_COPY_AS_ARRAY@ > > > > +#cmakedefine DBUS_WITH_GLIB 1 > > Just to check, this is conditional, right? We'll #define DBUS_WITH_GLIB 1 if > we have GLib, and leave it undefined if we don't? yes, see http://www.cmake.org/cmake/help/v3.2/command/configure_file.html?highlight=cmakedefine (In reply to Simon McVittie from comment #23) > (In reply to Ralf Habacker from comment #21) > > What about something like this, which compiles without any warnings on linux > > and windows > ... > > return (size_t)write(fd, buf, len); > > This makes a nice example of why adding casts without thinking about the > semantics is not the correct solution for warnings like this. Just because > the compiler shuts up about it does not mean the code is right. yes and this shows also that it is important that such warning should not be supressed. The initial developer would have seen the warning and would have been informed to choose a sign save implementation. :-) Created attachment 113956 [details] [review] Fix warning 'conversion to ‘long unsigned int’ from ‘WriteResult’ may change the sign of the result [-Wsign-conversion]'. Created attachment 113957 [details] [review] Fix warning: conversion to 'DWORD' from 'int' may change the sign of the result [-Wsign-conversion]. Created attachment 113958 [details] [review] Fix warning: variable '...' set but not used [-Wunused-but-set-variable]. Comment on attachment 113958 [details] [review] Fix warning: variable '...' set but not used [-Wunused-but-set-variable]. Review of attachment 113958 [details] [review]: ----------------------------------------------------------------- I need to refactor this patch, it has platform issues. Created attachment 113976 [details] [review] Fix of 'warning: unused variable ‘result‘ [-Wunused-variable]'. Created attachment 113977 [details] [review] Fix of 'warning: variable ‘..‘ set but not used [-Wunused-but-set-variable]'. Created attachment 113978 [details] [review] Fix of 'warning: variable ‘..‘ set but not used [-Wunused-but-set-variable]'. (update) was incomplete Created attachment 113980 [details] [review] Fix of 'warning: conversion to ‘dbus_bool_t‘ from ‘int‘ may change the sign of the result [-Wsign-conversion]'. Comment on attachment 113956 [details] [review] Fix warning 'conversion to ‘long unsigned int’ from ‘WriteResult’ may change the sign of the result [-Wsign-conversion]'. Review of attachment 113956 [details] [review]: ----------------------------------------------------------------- ::: tools/tool-common.c @@ +90,4 @@ > return FALSE; > } > > + size_t this_time = (size_t) res; I'd prefer to have a comment /* we now know res >= 0 */ and/or put it in an "else" block, to make it clearer that this cast is valid. Casts are essentially telling the compiler "shut up, I know what I'm doing" and they silence a lot of warnings (whether it's correct to do so or not), so it's important that they're obviously correct. Comment on attachment 113957 [details] [review] Fix warning: conversion to 'DWORD' from 'int' may change the sign of the result [-Wsign-conversion]. Review of attachment 113957 [details] [review]: ----------------------------------------------------------------- ::: tools/tool-common.c @@ +40,4 @@ > /* a hack to avoid having to depend on the static -util version of libdbus; > * it's useful for ancillary programs to be able to use the shared library */ > void > +tool_millisleep (unsigned int ms) This change is fine for now. We can remove tool_millisleep() altogether and use _dbus_sleep_milliseconds() instead, now that Bug #83115 is closed, but that isn't critical-path. Comment on attachment 113976 [details] [review] Fix of 'warning: unused variable ‘result‘ [-Wunused-variable]'. Review of attachment 113976 [details] [review]: ----------------------------------------------------------------- Fine Comment on attachment 113978 [details] [review] Fix of 'warning: variable ‘..‘ set but not used [-Wunused-but-set-variable]'. (update) Review of attachment 113978 [details] [review]: ----------------------------------------------------------------- ::: test/test-service.c @@ +401,5 @@ > DBusConnection *connection; > const char *name; > +#ifndef DBUS_WIN > + dbus_bool_t do_fork = FALSE; > +#endif Fine ::: tools/dbus-launch-win.c @@ -104,5 @@ > > -#ifdef AUTO_ACTIVATE_CONSOLE_WHEN_VERBOSE_MODE > - if (verbose) > - showConsole = 1; > -#endif OK if you don't want this feature any more - you're the D-Bus-on-Windows maintainer, so up to you to decide whether it is/was useful Comment on attachment 113980 [details] [review] Fix of 'warning: conversion to ‘dbus_bool_t‘ from ‘int‘ may change the sign of the result [-Wsign-conversion]'. Review of attachment 113980 [details] [review]: ----------------------------------------------------------------- I would have expected that the compiler should be able to infer that the only values these variables can possibly take are 0 and 1, resulting in no possibility of sign-changing... but using dbus_bool_t here is correct anyway, so this is fine. Created attachment 113985 [details] [review] Fix warning 'conversion to ‘long unsigned int’ from ‘WriteResult’ may change the sign of the result [-Wsign-conversion]'. (update) Created attachment 113987 [details] [review] Drop duplicated function tool_millisleep() and use dbus_sleep_milliseconds() instead. Comment on attachment 113976 [details] [review] Fix of 'warning: unused variable ‘result‘ [-Wunused-variable]'. committed to master Comment on attachment 113957 [details] [review] Fix warning: conversion to 'DWORD' from 'int' may change the sign of the result [-Wsign-conversion]. committed to master Comment on attachment 113985 [details] [review] Fix warning 'conversion to ‘long unsigned int’ from ‘WriteResult’ may change the sign of the result [-Wsign-conversion]'. (update) Review of attachment 113985 [details] [review]: ----------------------------------------------------------------- ok Comment on attachment 113987 [details] [review] Drop duplicated function tool_millisleep() and use dbus_sleep_milliseconds() instead. Review of attachment 113987 [details] [review]: ----------------------------------------------------------------- fine Comment on attachment 113987 [details] [review] Drop duplicated function tool_millisleep() and use dbus_sleep_milliseconds() instead. committed to master Comment on attachment 113985 [details] [review] Fix warning 'conversion to ‘long unsigned int’ from ‘WriteResult’ may change the sign of the result [-Wsign-conversion]'. (update) committed to master (In reply to Simon McVittie from comment #37) > Comment on attachment 113978 [details] [review] [review] > Fix of 'warning: variable ‘..‘ set but not used > [-Wunused-but-set-variable]'. (update) > > Review of attachment 113978 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: test/test-service.c > @@ +401,5 @@ > > DBusConnection *connection; > > const char *name; > > +#ifndef DBUS_WIN > > + dbus_bool_t do_fork = FALSE; > > +#endif > > Fine > > ::: tools/dbus-launch-win.c > @@ -104,5 @@ > > > > -#ifdef AUTO_ACTIVATE_CONSOLE_WHEN_VERBOSE_MODE > > - if (verbose) > > - showConsole = 1; > > -#endif > > OK if you don't want this feature any more - you're the D-Bus-on-Windows > maintainer, so up to you to decide whether it is/was useful seems not to be implemented completly, so it is okay. applied to master *** This bug has been marked as a duplicate of bug 93069 *** |
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.