Bug 27623 - should check each read()/write() for success
Summary: should check each read()/write() for success
Status: RESOLVED NOTABUG
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium normal
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords: love
: DEMO_2 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-13 18:25 UTC by Marcus Brinkmann
Modified: 2014-11-20 04:21 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Silence unused-return-value warnings on recent glibc. (1.53 KB, patch)
2010-04-13 18:25 UTC, Marcus Brinkmann
Details | Splinter Review

Description Marcus Brinkmann 2010-04-13 18:25:09 UTC
Created attachment 34981 [details] [review]
Silence unused-return-value warnings on recent glibc.

Recent glibc warn if result of read() or write() is unused.  Prefixing (void) is not enough, glibc insists that one does something with the value.  So I added dummy variables to hold the return result.

This patch should not affect users of older glibc versions.
Comment 1 Simon McVittie 2011-04-08 04:49:37 UTC
*** Bug 23453 has been marked as a duplicate of this bug. ***
Comment 2 Simon McVittie 2011-04-08 04:55:22 UTC
Review of attachment 34981 [details] [review]:

::: dbus/dbus-spawn.c
@@ +1080,3 @@
         {
           char b;
+          int res = read (sigchld_pipe[READ_END], &b, 1);

Recent gcc will complain that you're not doing anything with res.

In this specific case, it's a pipe-to-self and we don't actually want the data - we just want the side-effect of being woken up from the main loop. Adding "(void) res;", and a comment explaining why we don't care, seems most appropriate.

::: tools/dbus-launch.c
@@ +370,3 @@
+      res = write (1, bus_address, strlen (bus_address) + 1);
+      res = write (1, &bus_pid, sizeof bus_pid);
+      res = write (1, &bus_wid, sizeof bus_wid);

We should check these properly: the purpose of the warning is to get people checking return values so failure to read/write doesn't silently go unreported.

These should probably use do_write() (a local function in dbus-launch) instead of write(), to handle EINTR and die if we fail to write.
Comment 3 Arun Raghavan 2012-10-12 16:46:33 UTC
This causes a build failure on master:

main.c: In function 'signal_handler':
main.c:94:19: error: ignoring return value of 'write', declared with attribute warn_unused_result [-Werror=unused-result]
main.c:116:19: error: ignoring return value of 'write', declared with attribute warn_unused_result [-Werror=unused-result]
cc1: all warnings being treated as errors
Comment 4 Simon McVittie 2012-10-12 17:06:59 UTC
Patches welcome, or configure with CFLAGS=-Wno-error=unused-result.

If you want to fix this (yes please!), I think the reads and writes can be categorized into two classes:

1) gcc has actually spotted a bug

::: tools/dbus-launch.c
@@ +370,3 @@
+      res = write (1, bus_address, strlen (bus_address) + 1);
+      res = write (1, &bus_pid, sizeof bus_pid);
+      res = write (1, &bus_wid, sizeof bus_wid);

These should be checked, for instance via do_write().

2) false positive

I suspect the most correct way to deal with reads/writes that, having audited them, we still don't care about is to use an "if" with an empty body, which is a good place to put the comment justifying why it's OK to ignore the return. Something like this:

    if (read (sigchld_pipe[READ_END], &b, 1) != 1)
      {
        /* We really don't care what the byte was, or whether there
         * even was one: we're only calling read() for its side-effect
         * of draining the pipe.
         *
         * The 'if' is only here to silence -Werror=unused-result.
         */
      }

Scattering (void) casts around seems likely to stop working as gcc gets better at noticing people trying to trick it.

The one in main.c that Arun reported is another false positive: we're only writing to stderr with write() because it's async-signal-safe, and fprintf() isn't. If the write() fails, what are we going to do about it anyway, given that "complain to stderr" is no longer a useful action? I think the only answer we can give is "ignore it and hope for the best".
Comment 5 Simon McVittie 2013-08-27 16:41:38 UTC
Appears to have been fixed in master for 1.7.6.


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.