From c98b3545008d8e984ab456dcf79787418fcbfe13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 May 2019 13:46:55 +0200 Subject: [PATCH 1/5] network: remove redunant link name in message Fixes #12454. gcc was complaining that the link->ifname argument is NULL. Adding assert(link->ifname) right before the call has no effect. It seems that gcc is confused by the fact that log_link_warning_errno() internally calls log_object(), with link->ifname passed as the object. log_object() is also a macro and is does a check whether the passed object is NULL. So we have a check if something is NULL right next an unconditional use of it where it cannot be NULL. I think it's a bug in gcc. Anyway, we don't need to use link->ifname here. log_object() already prepends the object name to the message. --- src/network/networkd-link.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 533193ac93..6fc8294003 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -338,8 +338,7 @@ static int link_enable_ipv6(Link *link) { r = sysctl_write_ip_property_boolean(AF_INET6, link->ifname, "disable_ipv6", disabled); if (r < 0) - log_link_warning_errno(link, r, "Cannot %s IPv6 for interface %s: %m", - enable_disable(!disabled), link->ifname); + log_link_warning_errno(link, r, "Cannot %s IPv6: %m", enable_disable(!disabled)); else log_link_info(link, "IPv6 successfully %sd", enable_disable(!disabled)); From f1d553e9dfd56f95b7564dd20a0b56e6a0d6492c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 May 2019 14:15:46 +0200 Subject: [PATCH 2/5] shared/utmp-wtmp: avoid gcc warning about strncpy truncation The fact that strncpy does the truncation is the whole point here, and gcc shouldn't warn about this. We can avoid the warning and simplify the whole procedure by directly copying the interesting part. --- src/shared/utmp-wtmp.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index 4b134b6c8a..db4811b118 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -183,16 +183,14 @@ int utmp_put_reboot(usec_t t) { return write_entry_both(&store); } -_pure_ static const char *sanitize_id(const char *id) { +static void copy_suffix(char *buf, size_t buf_size, const char *src) { size_t l; - assert(id); - l = strlen(id); - - if (l <= sizeof(((struct utmpx*) NULL)->ut_id)) - return id; - - return id + l - sizeof(((struct utmpx*) NULL)->ut_id); + l = strlen(src); + if (l < buf_size) + strncpy(buf, src, buf_size); + else + memcpy(buf, src + l - buf_size, buf_size); } int utmp_put_init_process(const char *id, pid_t pid, pid_t sid, const char *line, int ut_type, const char *user) { @@ -207,8 +205,8 @@ int utmp_put_init_process(const char *id, pid_t pid, pid_t sid, const char *line init_timestamp(&store, 0); - /* ut_id needs only be nul-terminated if it is shorter than sizeof(ut_id) */ - strncpy(store.ut_id, sanitize_id(id), sizeof(store.ut_id)); + /* Copy the whole string if it fits, or just the suffix without the terminating NUL. */ + copy_suffix(store.ut_id, sizeof(store.ut_id), id); if (line) strncpy(store.ut_line, basename(line), sizeof(store.ut_line)); @@ -244,8 +242,8 @@ int utmp_put_dead_process(const char *id, pid_t pid, int code, int status) { setutxent(); - /* ut_id needs only be nul-terminated if it is shorter than sizeof(ut_id) */ - strncpy(lookup.ut_id, sanitize_id(id), sizeof(lookup.ut_id)); + /* Copy the whole string if it fits, or just the suffix without the terminating NUL. */ + copy_suffix(store.ut_id, sizeof(store.ut_id), id); found = getutxid(&lookup); if (!found) From 881a62debbac022046f350be5c3c7e909189a7e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 May 2019 14:56:01 +0200 Subject: [PATCH 3/5] scsi_serial: replace &foo[n] by foo+n --- src/udev/scsi_id/scsi_serial.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/udev/scsi_id/scsi_serial.c b/src/udev/scsi_id/scsi_serial.c index 481c426618..0abc8ca212 100644 --- a/src/udev/scsi_id/scsi_serial.c +++ b/src/udev/scsi_id/scsi_serial.c @@ -388,7 +388,7 @@ static int do_scsi_page0_inquiry(struct scsi_id_device *dev_scsi, int fd, * If the vendor id appears in the page assume the page is * invalid. */ - if (strneq((char *)&buffer[VENDOR_LENGTH], dev_scsi->vendor, VENDOR_LENGTH)) { + if (strneq((char*) buffer + VENDOR_LENGTH, dev_scsi->vendor, VENDOR_LENGTH)) { log_debug("%s: invalid page0 data", dev_scsi->kernel); return 1; } @@ -497,7 +497,7 @@ static int check_fill_0x83_id(struct scsi_id_device *dev_scsi, * included in the identifier. */ if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC) - if (prepend_vendor_model(dev_scsi, &serial[1]) < 0) + if (prepend_vendor_model(dev_scsi, serial + 1) < 0) return 1; i = 4; /* offset to the start of the identifier */ @@ -520,12 +520,12 @@ static int check_fill_0x83_id(struct scsi_id_device *dev_scsi, } } - strcpy(serial_short, &serial[s]); + strcpy(serial_short, serial + s); if (id_search->id_type == SCSI_ID_NAA && wwn != NULL) { - strncpy(wwn, &serial[s], 16); + strncpy(wwn, serial + s, 16); if (wwn_vendor_extension) - strncpy(wwn_vendor_extension, &serial[s + 16], 16); + strncpy(wwn_vendor_extension, serial + s + 16, 16); } return 0; @@ -619,8 +619,8 @@ static int do_scsi_page83_inquiry(struct scsi_id_device *dev_scsi, int fd, * one or a small number of descriptors. */ for (j = 4; j <= (unsigned)page_83[3] + 3; j += page_83[j + 3] + 4) { - retval = check_fill_0x83_id(dev_scsi, &page_83[j], - &id_search_list[id_ind], + retval = check_fill_0x83_id(dev_scsi, page_83 + j, + id_search_list + id_ind, serial, serial_short, len, wwn, wwn_vendor_extension, tgpt_group); @@ -731,7 +731,7 @@ static int do_scsi_page80_inquiry(struct scsi_id_device *dev_scsi, int fd, len = buf[3]; if (serial) { serial[0] = 'S'; - ser_ind = prepend_vendor_model(dev_scsi, &serial[1]); + ser_ind = prepend_vendor_model(dev_scsi, serial + 1); if (ser_ind < 0) return 1; ser_ind++; /* for the leading 'S' */ @@ -739,7 +739,7 @@ static int do_scsi_page80_inquiry(struct scsi_id_device *dev_scsi, int fd, serial[ser_ind] = buf[i]; } if (serial_short) { - memcpy(serial_short, &buf[4], len); + memcpy(serial_short, buf + 4, len); serial_short[len] = '\0'; } return 0; From 6695c200bd5f0cbe65647fcdafa2f1dbed4b6a64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 May 2019 15:10:58 +0200 Subject: [PATCH 4/5] shared/utmp-wtmp: silence gcc warning about strncpy truncation Unfortunately the warning must be known, or otherwise the pragma generates a warning or an error. So let's do a meson check for it. Is it worth doing this to silence the warning? I think so, because apparently the warning was already emitted by gcc-8.1, and with the recent push in gcc to catch more such cases, we'll most likely only get more of those. --- meson.build | 3 +++ src/basic/macro.h | 9 +++++++++ src/basic/string-util.h | 6 ++++++ src/shared/utmp-wtmp.c | 2 +- 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/meson.build b/meson.build index 24ef643d44..b5d06e4226 100644 --- a/meson.build +++ b/meson.build @@ -411,11 +411,14 @@ endif cpp = ' '.join(cc.cmd_array()) + ' -E' +has_wstringop_truncation = cc.has_argument('-Wstringop-truncation') + ##################################################################### # compilation result tests conf.set('_GNU_SOURCE', true) conf.set('__SANE_USERSPACE_TYPES__', true) +conf.set10('HAVE_WSTRINGOP_TRUNCATION', has_wstringop_truncation) conf.set('SIZEOF_PID_T', cc.sizeof('pid_t', prefix : '#include ')) conf.set('SIZEOF_UID_T', cc.sizeof('uid_t', prefix : '#include ')) diff --git a/src/basic/macro.h b/src/basic/macro.h index 1971e912db..ae8907db04 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -105,6 +105,15 @@ _Pragma("GCC diagnostic push"); \ _Pragma("GCC diagnostic ignored \"-Wincompatible-pointer-types\"") +#if HAVE_WSTRINGOP_TRUNCATION +# define DISABLE_WARNING_STRINGOP_TRUNCATION \ + _Pragma("GCC diagnostic push"); \ + _Pragma("GCC diagnostic ignored \"-Wstringop-truncation\"") +#else +# define DISABLE_WARNING_STRINGOP_TRUNCATION \ + _Pragma("GCC diagnostic push") +#endif + #define REENABLE_WARNING \ _Pragma("GCC diagnostic pop") diff --git a/src/basic/string-util.h b/src/basic/string-util.h index b23f4c8341..a630856236 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -212,6 +212,12 @@ static inline size_t strlen_ptr(const char *s) { return strlen(s); } +DISABLE_WARNING_STRINGOP_TRUNCATION; +static inline void strncpy_exact(char *buf, const char *src, size_t buf_len) { + strncpy(buf, src, buf_len); +} +REENABLE_WARNING; + /* Like startswith(), but operates on arbitrary memory blocks */ static inline void *memory_startswith(const void *p, size_t sz, const char *token) { size_t n; diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index db4811b118..646f449821 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -209,7 +209,7 @@ int utmp_put_init_process(const char *id, pid_t pid, pid_t sid, const char *line copy_suffix(store.ut_id, sizeof(store.ut_id), id); if (line) - strncpy(store.ut_line, basename(line), sizeof(store.ut_line)); + strncpy_exact(store.ut_line, line, sizeof(store.ut_line)); r = write_entry_both(&store); if (r < 0) From 099c77fd5ff835614dea8dc11c57f6d44f77d9ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 7 May 2019 15:58:29 +0200 Subject: [PATCH 5/5] scsi_serial: replace some crazy strncpy() calls by strnlen() gcc was warning about strncpy() leaving an unterminated string. In this case, it was correct. The code was doing strncpy()+strncat()+strlen() essentially to determine if the strings have expected length. If the length was correct, a buffer overread was performed (or at least some garbage bytes were used from the uninitialized part of the buffer). Let's do the length check first and then only copy stuff if everything agrees. For some reason the function was called "prepend", when it obviously does an "append". --- src/udev/scsi_id/scsi_serial.c | 36 ++++++++++++++-------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/udev/scsi_id/scsi_serial.c b/src/udev/scsi_id/scsi_serial.c index 0abc8ca212..e1940e7d38 100644 --- a/src/udev/scsi_id/scsi_serial.c +++ b/src/udev/scsi_id/scsi_serial.c @@ -396,27 +396,21 @@ static int do_scsi_page0_inquiry(struct scsi_id_device *dev_scsi, int fd, return 0; } -/* - * The caller checks that serial is long enough to include the vendor + - * model. - */ -static int prepend_vendor_model(struct scsi_id_device *dev_scsi, char *serial) { - int ind; +static int append_vendor_model( + const struct scsi_id_device *dev_scsi, + char buf[static VENDOR_LENGTH + MODEL_LENGTH]) { - strncpy(serial, dev_scsi->vendor, VENDOR_LENGTH); - strncat(serial, dev_scsi->model, MODEL_LENGTH); - ind = strlen(serial); - - /* - * This is not a complete check, since we are using strncat/cpy - * above, ind will never be too large. - */ - if (ind != (VENDOR_LENGTH + MODEL_LENGTH)) + if (strnlen(dev_scsi->vendor, VENDOR_LENGTH) != VENDOR_LENGTH) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), - "%s: expected length %d, got length %d", - dev_scsi->kernel, - (VENDOR_LENGTH + MODEL_LENGTH), ind); - return ind; + "%s: bad vendor string \"%s\"", + dev_scsi->kernel, dev_scsi->vendor); + if (strnlen(dev_scsi->model, MODEL_LENGTH) != MODEL_LENGTH) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: bad model string \"%s\"", + dev_scsi->kernel, dev_scsi->model); + memcpy(buf, dev_scsi->vendor, VENDOR_LENGTH); + memcpy(buf + VENDOR_LENGTH, dev_scsi->model, MODEL_LENGTH); + return VENDOR_LENGTH + MODEL_LENGTH; } /* @@ -497,7 +491,7 @@ static int check_fill_0x83_id(struct scsi_id_device *dev_scsi, * included in the identifier. */ if (id_search->id_type == SCSI_ID_VENDOR_SPECIFIC) - if (prepend_vendor_model(dev_scsi, serial + 1) < 0) + if (append_vendor_model(dev_scsi, serial + 1) < 0) return 1; i = 4; /* offset to the start of the identifier */ @@ -731,7 +725,7 @@ static int do_scsi_page80_inquiry(struct scsi_id_device *dev_scsi, int fd, len = buf[3]; if (serial) { serial[0] = 'S'; - ser_ind = prepend_vendor_model(dev_scsi, serial + 1); + ser_ind = append_vendor_model(dev_scsi, serial + 1); if (ser_ind < 0) return 1; ser_ind++; /* for the leading 'S' */