[PULL] button mapping fix and unconstify patches
Keith Packard
keithp at keithp.com
Sat Mar 22 13:55:40 PDT 2014
Keith Packard <keithp at keithp.com> 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 <keithp at keithp.com>
> 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 <keithp at keithp.com>
> ---
> 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
>
>
> --
> keith.packard at intel.com
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
--
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140322/1c9f997f/attachment-0001.sig>
More information about the xorg-devel
mailing list