Gaetan Nadon <[email protected]> writes: > On 14-02-11 12:08 AM, Keith Packard wrote: >> It's not just the GCC version, unfortunately. I have a pile of versions, >> and cannot generate any of the (useful) warnings that you have managed >> to elicit from the compiler. > I tried Ubuntu 13-10 with gcc 4.8.2. The 32-bit version > > 35 -Wunused-result
Oh, right, I remember this from fontconfig -- ubuntu sets
_FORTIFY_SOURCE by default. I'd love to use this in the X server by
default, but I'm concerned that we'll end up breaking builds on ubuntu
if we unilaterally define it. Can you see what your build does with
$ make CC="gcc -D_FORTIFY_SOURCE=2"
if it works, then we'd be find setting it for all gcc builds, otherwise
we'll need magic.
On my machine, I'm now getting 37 warnings (I'm also building kdrive
which has two warnings).
Yay! I can fix these easily now.
> 0 -Wpointer-arith (as expected)
> 0 -Wformat (you have 1)
I'm only a -Wformat issue in 64-bits, but 32-bits because (sizeof (int) ==
sizeof (long))
> 1 -Wunused-function
>
> On the 64 bit with gcc 4.6.3:
>
> 35 -Wunused-result
> 2 -Wpointer-arith
> 2 -Wformat
> 1 -Wunused-function
Sounds like you're a lot closer on 13-10 than 12-10. That's good news!
Here's a patch which cleans up all of the -Wunused-result warnings I'm
getting with -D_FORTIFY_SOURCE=2. I haven't tried running this server...
From 65b855b39370ce5dd49501fa4d8ed8d8e4c52f9e Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]> Date: Tue, 11 Feb 2014 19:44:03 -0800 Subject: [PATCH] Clean up warnings from -D_FORTIFY_SOURCE=2 New glibc versions have extra error checking available, warning if return values from some functions are ignored and doing additional argument checking on strings. These are useful, and Ubuntu turns them on. With the sole exception of writes in the logging function, these patches check the indicated return values and do something reasonable in each case. Signed-off-by: Keith Packard <[email protected]> --- hw/kdrive/linux/linux.c | 16 +++++++++++----- hw/kdrive/src/kdrive.c | 5 ++++- include/os.h | 13 +++++++++++++ os/connection.c | 4 ++-- os/log.c | 4 ++-- os/utils.c | 18 +++++++++++++++++- test/signal-logging.c | 4 +++- xkb/xkmread.c | 10 ++++++++-- 8 files changed, 60 insertions(+), 14 deletions(-) diff --git a/hw/kdrive/linux/linux.c b/hw/kdrive/linux/linux.c index 6284de5..d3fab4b 100644 --- a/hw/kdrive/linux/linux.c +++ b/hw/kdrive/linux/linux.c @@ -30,6 +30,7 @@ #include <linux/kd.h> #include <sys/stat.h> #include <sys/ioctl.h> +#include <stdbool.h> #include <X11/keysym.h> #include <linux/apm_bios.h> @@ -62,7 +63,7 @@ LinuxVTRequest(int sig) } /* Check before chowning -- this avoids touching the file system */ -static void +static bool LinuxCheckChown(const char *file) { struct stat st; @@ -70,11 +71,14 @@ LinuxCheckChown(const char *file) __gid_t g; if (stat(file, &st) < 0) - return; + return false; u = getuid(); g = getgid(); - if (st.st_uid != u || st.st_gid != g) - chown(file, u, g); + if (st.st_uid != u || st.st_gid != g) { + if (chown(file, u, g) < 0) + return false; + } + return true; } static int @@ -110,7 +114,9 @@ LinuxInit(void) } /* change ownership of the vt */ - LinuxCheckChown(vtname); + if (!LinuxCheckChown(vtname)) { + FatalError("LinuxInit: Can't switch VT %s ownership (%s)\n", vtname, strerror(errno)); + } /* * the current VT device we're running on is not "console", we want diff --git a/hw/kdrive/src/kdrive.c b/hw/kdrive/src/kdrive.c index 8eb8cd0..541a7e4 100644 --- a/hw/kdrive/src/kdrive.c +++ b/hw/kdrive/src/kdrive.c @@ -118,10 +118,13 @@ KdDoSwitchCmd(const char *reason) { if (kdSwitchCmd) { char *command; + int ret; if (asprintf(&command, "%s %s", kdSwitchCmd, reason) == -1) return; - system(command); + ret = system(command); + if (ret != 0) + ErrorF("Switch command \"%s %s\" failed: %d\n", kdSwitchCmd, reason, ret); free(command); } } diff --git a/include/os.h b/include/os.h index e5f86d6..0084a99 100644 --- a/include/os.h +++ b/include/os.h @@ -280,6 +280,19 @@ Xstrdup(const char *s); extern _X_EXPORT char * XNFstrdup(const char *s); +/* + * This function write(2)s data, exiting with a fatal error if the write fails + */ +extern _X_EXPORT void +XNFwrite(int fd, const void *buf, size_t len); + +/* + * This function write(2)s data, ignoring any errors. It's used for logging, + * where there's really not much we can do if the log writes fail... + */ +extern _X_EXPORT void +XIGNOREwrite(int fd, const void *buf, size_t len); + /* Include new X*asprintf API */ #include "Xprintf.h" diff --git a/os/connection.c b/os/connection.c index ddf4f0a..d7505e4 100644 --- a/os/connection.c +++ b/os/connection.c @@ -352,8 +352,8 @@ NotifyParentProcess(void) { #if !defined(WIN32) if (dynamic_display[0]) { - write(displayfd, dynamic_display, strlen(dynamic_display)); - write(displayfd, "\n", 1); + XNFwrite(displayfd, dynamic_display, strlen(dynamic_display)); + XNFwrite(displayfd, "\n", 1); close(displayfd); } if (RunFromSmartParent) { diff --git a/os/log.c b/os/log.c index 8deb810..ee4910b 100644 --- a/os/log.c +++ b/os/log.c @@ -489,11 +489,11 @@ LogSWrite(int verb, const char *buf, size_t len, Bool end_line) static Bool newline = TRUE; if (verb < 0 || logVerbosity >= verb) - write(2, buf, len); + XIGNOREwrite(2, buf, len); if (verb < 0 || logFileVerbosity >= verb) { if (inSignalContext && logFileFd >= 0) { - write(logFileFd, buf, len); + XIGNOREwrite(logFileFd, buf, len); #ifndef WIN32 if (logFlush && logSync) fsync(logFileFd); diff --git a/os/utils.c b/os/utils.c index 497779b..f45abdd 100644 --- a/os/utils.c +++ b/os/utils.c @@ -313,7 +313,7 @@ LockServer(void) if (lfd < 0) FatalError("Could not create lock file in %s\n", tmp); snprintf(pid_str, sizeof(pid_str), "%10ld\n", (long) getpid()); - (void) write(lfd, pid_str, 11); + XNFwrite(lfd, pid_str, 11); (void) fchmod(lfd, 0444); (void) close(lfd); @@ -1180,6 +1180,22 @@ XNFstrdup(const char *s) } void +XNFwrite(int fd, const void *data, size_t len) +{ + if (write (fd, data, len) != len) + FatalError("XNFwrite: failed to write (%s)\n", strerror(errno)); +} + +void +XIGNOREwrite(int fd, const void *data, size_t len) +{ + ssize_t ret; + + ret = write(fd, data, len); + (void) ret; +} + +void SmartScheduleStopTimer(void) { #ifdef SMART_SCHEDULE_POSSIBLE diff --git a/test/signal-logging.c b/test/signal-logging.c index d894373..0f0d66c 100644 --- a/test/signal-logging.c +++ b/test/signal-logging.c @@ -170,6 +170,7 @@ static void logging_format(void) char read_buf[2048]; char *logmsg; uintptr_t ptr; + char *fgets_result; /* set up buf to contain ".....end" */ memset(buf, '.', sizeof(buf)); @@ -179,7 +180,8 @@ static void logging_format(void) assert(f = fopen(log_file_path, "r")); #define read_log_msg(msg) \ - fgets(read_buf, sizeof(read_buf), f); \ + fgets_result = fgets(read_buf, sizeof(read_buf), f); \ + assert(fgets_result != NULL); \ msg = strchr(read_buf, ']') + 2; /* advance past [time.stamp] */ /* boring test message */ diff --git a/xkb/xkmread.c b/xkb/xkmread.c index 258bb91..93b9f49 100644 --- a/xkb/xkmread.c +++ b/xkb/xkmread.c @@ -1204,7 +1204,10 @@ XkmReadTOC(FILE * file, xkmFileInfo * file_info, int max_toc, } return 0; } - fread(file_info, SIZEOF(xkmFileInfo), 1, file); + if (fread(file_info, SIZEOF(xkmFileInfo), 1, file) != 1) { + _XkbLibError(_XkbErrEmptyFile, "XkmReadTOC", 0); + return 0; + } size_toc = file_info->num_toc; if (size_toc > max_toc) { DebugF("Warning! Too many TOC entries; last %d ignored\n", @@ -1212,7 +1215,10 @@ XkmReadTOC(FILE * file, xkmFileInfo * file_info, int max_toc, size_toc = max_toc; } for (i = 0; i < size_toc; i++) { - fread(&toc[i], SIZEOF(xkmSectionInfo), 1, file); + if (fread(&toc[i], SIZEOF(xkmSectionInfo), 1, file) != 1) { + _XkbLibError(_XkbErrBadLength, "XkmReadTOC", 0); + return 0; + } } return 1; } -- 1.9.rc1
-- [email protected]
pgpA5mcI6Xw2P.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
