Keith Packard <[email protected]> writes: I sent this patch out last month to fix warnings when -D_FORTIFY_SOURCE=2 was added to the build. I think Gaetan tried it and didn't have any trouble, but I'd love to get some actual review so we can merge it in. I believe it fixes several *actual bugs*, which makes it significantly different from most of my previous warning work.
Help would be appreciated; I'm currently building with this turned on to remind me that this hasn't been done yet... -keith > 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] > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel -- [email protected]
pgpgwqY1r3Bxe.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
