From 18ece5f9cc147999e57ed1bf139bcbd056185deb Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 02:48:24 -0500 Subject: Fix buffer overflow in cat_with Account for separators when computing the buffer size in cat_width in order to avoid overflowing the buffer. Previously, cat_width only accounted for the lengths of the strings being catenated, but not the separators, which could result in buffer overflows. For example, trace the execution of the following invocation of cat_with: char s[5] = "xxxx"; cat_with(' ', s, s, s, s, s, s, s, s); cat_with initialises size and length and allocates an initial buffer: size = 32; /* size = 32 */ length = 0; /* length = 0 */ buf = calloc(1, size + 1); /* allocated 33 bytes for buf */ The first iteration of the main loop in cat_with catenates the first string and a separator but fails to account for the separator in length: length += strlen(curr); /* length = 4 */ while (length + 2 > size) /* 4 + 2 > 32 is false */ strcat(buf, curr); /* written 5 bytes to buf */ strcat(buf, split); /* written 6 bytes to buf */ Now we have written the string "xxxx " (6 bytes including NUL) to buf, but length only counts the 4 bytes "xxxx" (excluding NUL). In successive iterations, the discrepancy between the value in length and the actual length of the string in the buffer grows: length += strlen(curr); /* length = 8 */ while (length + 2 > size) /* 8 + 2 > 32 is false */ strcat(buf, curr); /* written 10 bytes to buf */ strcat(buf, split); /* written 11 bytes to buf */ Now we have written "xxxx xxxx " (11 bytes incl. NUL). length += strlen(curr); /* length = 12 */ while (length + 2 > size) /* 12 + 2 > 32 is false */ strcat(buf, curr); /* written 15 bytes to buf */ strcat(buf, split); /* written 16 bytes to buf */ Now we have written "xxxx xxxx xxxx " (16 bytes incl. NUL). length += strlen(curr); /* length = 16 */ while (length + 2 > size) /* 16 + 2 > 32 is false */ strcat(buf, curr); /* written 20 bytes to buf */ strcat(buf, split); /* written 21 bytes to buf */ Now we have written "xxxx xxxx xxxx xxxx " (21 bytes incl. NUL). length += strlen(curr); /* length = 20 */ while (length + 2 > size) /* 20 + 2 > 32 is false */ strcat(buf, curr); /* written 25 bytes to buf */ strcat(buf, split); /* written 26 bytes to buf */ Now we have written "xxxx xxxx xxxx xxxx xxxx " (26 bytes incl. NUL). length += strlen(curr); /* length = 24 */ while (length + 2 > size) /* 24 + 2 > 32 is false */ strcat(buf, curr); /* written 30 bytes to buf */ strcat(buf, split); /* written 31 bytes to buf */ Now we have written "xxxx xxxx xxxx xxxx xxxx xxxx " (31 bytes incl. NUL). Eventually, the string can grow beyond the buffer before length grows large enough to cause a re-allocation: length += strlen(curr); /* length = 28 */ while (length + 2 > size) /* 28 + 2 > 32 is false */ strcat(buf, curr); /* written 35 bytes to buf */ strcat(buf, split); /* written 36 bytes to buf */ Note that we have not increased size or re-allocated the buffer, and so we have written "xxxx xxxx xxxx xxxx xxxx xxxx xxxx" (35 bytes incl. NUL) to a buffer for which we have allocated only 33 bytes. --- brightnessctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 355fc86..60ca3a4 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -463,8 +463,10 @@ char *_cat_with(char c, ...) { while (length + 2 > size) buf = realloc(buf, size *= 2); strcat(buf, curr); - if ((curr = va_arg(va, char*))) + if ((curr = va_arg(va, char*))) { + length++; strcat(buf, split); + } } return buf; } -- cgit v1.2.3 From caf20af422a7fc14c0d6eaa9cc330066b447a968 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 03:08:17 -0500 Subject: Set dev_name to default device name If no device is specified with the --device flag, set dev_name to the id of the first device. Before, if the --device flag is not used to specify the default device, the dev pointer was set to first device found (see commit c9140d1e5070dae2746d3b70c0390075bd5380f6), but the dev_name pointer was left set to NULL. As a result, some error messages did not print the name of the default device: % brightnessctl -s Error saving device data: No such file or directory Could not save data for device '(null)'. Device 'intel_backlight' of class 'backlight': Current brightness: 1000 (20%) Max brightness: 4794 % brightnessctl -s -m Error saving device data: No such file or directory Could not save data for device '(null)'. intel_backlight,backlight,1000,20%,4794 % After this commit, all error messages have the correct name: % brightnessctl -s Error saving device data: No such file or directory Could not save data for device 'intel_backlight'. Device 'intel_backlight' of class 'backlight': Current brightness: 1000 (20%) Max brightness: 4794 % brightnessctl -s -m Error saving device data: No such file or directory Could not save data for device 'intel_backlight'. intel_backlight,backlight,1000,20%,4794 % --- brightnessctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 60ca3a4..d6170e9 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -151,6 +151,8 @@ int main(int argc, char **argv) { return list_devices(devs); } dev_name = p.device; + if (!dev_name) + dev_name = devs[0]->id; if (argc == 0) p.operation = GET; else switch (argv[0][0]) { @@ -242,8 +244,6 @@ int parse_value(struct value *val, char *str) { struct device *find_device(struct device **devs, char *name) { struct device *dev; - if (!name) - return *devs; while ((dev = *(devs++))) if (!strcmp(dev->id, name)) return dev; -- cgit v1.2.3 From 8d17af9cc111d50a8b0be9ff2f35a9b333290781 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 03:23:43 -0500 Subject: -r/--restore requires root, same as set The -r/--restore flag requires root, the same as the set verb does, so print an error message if either the flag or the verb is used without running as root. --- brightnessctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index d6170e9..86a0e21 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -172,7 +172,7 @@ int main(int argc, char **argv) { fail("Invalid value given"); if (!(dev = find_device(devs, dev_name))) fail("Device '%s' not found.\n", dev_name); - if (p.operation == SET && !p.pretend && geteuid()) + if ((p.operation == SET || p.restore) && !p.pretend && geteuid()) fail("You need to run this program as root to be able to modify values!\n"); if (p.save) if (save_device_data(dev)) -- cgit v1.2.3 From 0a6ee486b4a1171adc0fde02a880f99d5d9a2ebb Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 03:27:09 -0500 Subject: Fix logic for directory creation when saving state When creating the /tmp/brightnessctl directory or its subdirectories, clear errno before calling mkdir, and call stat a second time after mkdir. Previously, errno was not cleared, so even if the error was handled (by creating the missing directory with mkdir), subsequent error handling code read the old errno value from the failed stat call and printed an error message. Additionally, because we did not call stat again after calling mkdir, the stat buffer had outdated information, and so S_ISDIR returned a false negative. As a result of these defects, invoking brightnessctl -s when /tmp/brightnessctl did not exist would create /tmp/brightnessctl and fail. Invoking brightnessctl -s a second time would create /tmp/brightnessctl/backlight and fail. Invoking the command a third time would succeed and write the state file under /tmp/brightnessctl/backlight/. After this commit, brightnessctl -s succeeds the first time. --- brightnessctl.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 86a0e21..3f7cfc1 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -395,8 +395,11 @@ int save_device_data(struct device *dev) { if (stat(c_path, &sb)) { if (errno != ENOENT) goto fail; + errno = 0; if (mkdir(c_path, 0777)) goto fail; + if (stat(c_path, &sb)) + goto fail; } if (!S_ISDIR(sb.st_mode)) goto fail; @@ -442,9 +445,12 @@ static int ensure_run_dir() { if (stat(run_dir, &sb)) { if (errno != ENOENT) return 0; + errno = 0; if (mkdir(run_dir, 0777)) { return 0; } + if (stat(run_dir, &sb)) + return 0; } return S_ISDIR(sb.st_mode); } -- cgit v1.2.3 From eff9d66ba6af627543a37ea36567d9ce6d0e28a8 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 03:46:02 -0500 Subject: Do not write NUL to state file Do not write a trailing NUL byte to the state file. Before: % brightnessctl -ms intel_backlight,backlight,1000,20%,4794 % od -t x1z /tmp/brightnessctl/backlight/intel_backlight 0000000 31 30 30 30 00 >1000.< 0000005 % After: % brightnessctl -ms intel_backlight,backlight,1000,20%,4794 % od -t x1z /tmp/brightnessctl/backlight/intel_backlight 0000000 31 30 30 30 >1000< 0000004 % Note that restore still works: % brightnessctl -mr intel_backlight,backlight,1000,20%,4794 % --- brightnessctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 3f7cfc1..1eee6c6 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -405,7 +405,7 @@ int save_device_data(struct device *dev) { goto fail; if (!(fp = fopen(d_path, "w"))) goto fail; - if (fwrite(c, 1, s + 1, fp) < s + 1) + if (fwrite(c, 1, s, fp) < s) errno = -1; fclose(fp); fail: -- cgit v1.2.3 From 0cedae398a108e287edb440ddcf877fb58bcb272 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sat, 24 Dec 2016 03:57:47 -0500 Subject: Clean up error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make several changes to error handling: • Check for negative values from sprintf, and treat them as errors. • Check for errors from fscanf both by comparing its return value against EOF and by checking ferror. • Avoiding calling perror with errno set to -1. Either set a meaningful value, such as EINVAL, or use fprintf if errno cannot be expected to have a meaningful value. • Instead of re-using errno for internal error handling, use a new variable to indicate errors where it makes sense to do so. • In read_device, print more specific error messages if we fail to read a device's current or max brightness, and print an error message immediately if a read fails, instead of only maybe at the end of read_device. --- brightnessctl.c | 52 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 16 deletions(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 1eee6c6..55352d2 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -292,8 +292,8 @@ int write_device(struct device *d) { char c[16]; size_t s = sprintf(c, "%u", d->curr_brightness); errno = 0; - if (!s) { - errno = -1; + if (s <= 0) { + errno = EINVAL; goto fail; } if ((f = fopen(dir_child(device_path(d), "brightness"), "w"))) { @@ -314,7 +314,7 @@ int read_device(struct device *d, char *class, char *id) { DIR *dirp; FILE *f; char *dev_path; - int done_read = 0; + int error = 0; struct dirent *ent; d->class = strdup(class); d->id = strdup(id); @@ -326,14 +326,28 @@ int read_device(struct device *d, char *class, char *id) { continue; if (!strcmp(ent->d_name, "brightness")) { if ((f = fopen(dir_child(dev_path, ent->d_name), "r"))) { - done_read += fscanf(f, "%u", &d->curr_brightness); + clearerr(f); + if (fscanf(f, "%u", &d->curr_brightness) == EOF) { + fprintf(stderr, "End-of-file reading brightness of device '%s'.", d->id); + error++; + } else if (ferror(f)) { + fprintf(stderr, "Error reading brightness of device '%s': %s.", d->id, strerror(errno)); + error++; + } fclose(f); } else goto fail; } if (!strcmp(ent->d_name, "max_brightness")) { if ((f = fopen(dir_child(dev_path, ent->d_name), "r"))) { - done_read += fscanf(f, "%u", &d->max_brightness); + clearerr(f); + if (fscanf(f, "%u", &d->max_brightness) == EOF) { + fprintf(stderr, "End-of-file reading max brightness of device '%s'.", d->id); + error++; + } else if (ferror(f)) { + fprintf(stderr, "Error reading max brightness of device '%s': %s.", d->id, strerror(errno)); + error++; + } fclose(f); } else goto fail; @@ -342,11 +356,11 @@ int read_device(struct device *d, char *class, char *id) { errno = 0; fail: closedir(dirp); - if (!errno && !done_read) - errno = -1; - if (errno) + if (errno) { perror("Error reading device"); - return errno; + error++; + } + return error; } int read_class(struct device **devs, char *class) { @@ -385,9 +399,11 @@ int save_device_data(struct device *dev) { char *c_path = dir_child(run_dir, dev->class); char *d_path = dir_child(c_path, dev->id); FILE *fp; + int error = 0; errno = 0; - if (!s) { - errno = -1; + if (s <= 0) { + fprintf(stderr, "Error converting device data."); + error++; goto fail; } if (!ensure_run_dir()) @@ -405,15 +421,19 @@ int save_device_data(struct device *dev) { goto fail; if (!(fp = fopen(d_path, "w"))) goto fail; - if (fwrite(c, 1, s, fp) < s) - errno = -1; + if (fwrite(c, 1, s, fp) < s) { + fprintf(stderr, "Error writing to '%s'.\n", d_path); + error++; + } fclose(fp); fail: free(c_path); free(d_path); - if (errno) + if (errno) { perror("Error saving device data"); - return errno; + error++; + } + return error; } int restore_device_data(struct device *dev) { @@ -429,7 +449,7 @@ int restore_device_data(struct device *dev) { fclose(fp); dev->curr_brightness = strtol(buf, &end, 10); if (end == buf) - errno = -1; + errno = EINVAL; fail: if (errno) { perror("Error restoring device data"); -- cgit v1.2.3 From 4aa6d8fff1e1e034c1a73d02d3515df36b93bbaf Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Sun, 15 Jan 2017 02:18:49 -0500 Subject: Print error if run dir is not a directory Print an error message if the run directory or the device-class subdirectory exists but is not a directory. Before: % rm -rf /tmp/brightnessctl % touch /tmp/brightnessctl % brightnessctl -s Device 'intel_backlight' of class 'backlight': Current brightness: 1000 (20%) Max brightness: 4794 % file /tmp/brightnessctl /tmp/brightnessctl: empty % After: % brightnessctl -s Error saving device data: Not a directory Could not save data for device 'intel_backlight'. Device 'intel_backlight' of class 'backlight': Current brightness: 1000 (20%) Max brightness: 4794 % --- brightnessctl.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'brightnessctl.c') diff --git a/brightnessctl.c b/brightnessctl.c index 55352d2..cbfe410 100644 --- a/brightnessctl.c +++ b/brightnessctl.c @@ -417,8 +417,10 @@ int save_device_data(struct device *dev) { if (stat(c_path, &sb)) goto fail; } - if (!S_ISDIR(sb.st_mode)) + if (!S_ISDIR(sb.st_mode)) { + errno = ENOTDIR; goto fail; + } if (!(fp = fopen(d_path, "w"))) goto fail; if (fwrite(c, 1, s, fp) < s) { @@ -472,7 +474,11 @@ static int ensure_run_dir() { if (stat(run_dir, &sb)) return 0; } - return S_ISDIR(sb.st_mode); + if (!S_ISDIR(sb.st_mode)) { + errno = ENOTDIR; + return 0; + } + return 1; } char *_cat_with(char c, ...) { -- cgit v1.2.3