From 88bef035149080be5a83f90d91d5b13cec9749e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 11 Oct 2013 19:33:13 -0400 Subject: [PATCH] Never call qsort on potentially NULL arrays This extends 62678ded 'efi: never call qsort on potentially NULL arrays' to all other places where qsort is used and it is not obvious that the count is non-zero. --- src/analyze/systemd-analyze.c | 2 +- src/cgtop/cgtop.c | 2 +- src/core/namespace.c | 38 ++++++++++++++++++++------------------ src/journal/catalog.c | 2 +- src/journal/journal-file.c | 2 +- src/journal/journal-vacuum.c | 3 +-- src/journal/journalctl.c | 2 +- src/libsystemd-bus/bus-match.c | 2 +- src/libudev/libudev-enumerate.c | 2 +- src/nss-myhostname/netlink.c | 3 ++- src/readahead/readahead-collect.c | 39 ++++++++++++++++++++++----------------- src/shared/cgroup-show.c | 2 ++ src/shared/conf-files.c | 2 +- src/shared/efivars.c | 3 +-- src/shared/fileio.c | 1 + src/shared/util.h | 12 ++++++++++++ src/systemctl/systemctl.c | 10 +++++----- 17 files changed, 74 insertions(+), 53 deletions(-) diff --git a/src/analyze/systemd-analyze.c b/src/analyze/systemd-analyze.c index 27d063c..a4f15eb 100644 --- a/src/analyze/systemd-analyze.c +++ b/src/analyze/systemd-analyze.c @@ -768,7 +768,7 @@ static int list_dependencies_one(DBusConnection *bus, const char *name, unsigned if (r < 0) return r; - qsort(deps, strv_length(deps), sizeof (char*), list_dependencies_compare); + qsort_safe(deps, strv_length(deps), sizeof (char*), list_dependencies_compare); r = acquire_boot_times(bus, &boot); if (r < 0) diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index cacf705..293a211 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -461,7 +461,7 @@ static int display(Hashmap *a) { if (g->n_tasks_valid || g->cpu_valid || g->memory_valid || g->io_valid) array[n++] = g; - qsort(array, n, sizeof(Group*), group_compare); + qsort_safe(array, n, sizeof(Group*), group_compare); /* Find the longest names in one run */ for (j = 0; j < n; j++) { diff --git a/src/core/namespace.c b/src/core/namespace.c index 16b132b..936f368 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -222,7 +222,7 @@ int setup_namespace(char** read_write_dirs, strv_length(read_only_dirs) + strv_length(inaccessible_dirs) + (private_tmp ? 2 : 0); - BindMount *m, *mounts; + BindMount *m, *mounts = NULL; int r = 0; if (!mount_flags) @@ -231,27 +231,29 @@ int setup_namespace(char** read_write_dirs, if (unshare(CLONE_NEWNS) < 0) return -errno; - m = mounts = (BindMount *) alloca(n * sizeof(BindMount)); - if ((r = append_mounts(&m, read_write_dirs, READWRITE)) < 0 || - (r = append_mounts(&m, read_only_dirs, READONLY)) < 0 || - (r = append_mounts(&m, inaccessible_dirs, INACCESSIBLE)) < 0) - return r; + if (n) { + m = mounts = (BindMount *) alloca(n * sizeof(BindMount)); + if ((r = append_mounts(&m, read_write_dirs, READWRITE)) < 0 || + (r = append_mounts(&m, read_only_dirs, READONLY)) < 0 || + (r = append_mounts(&m, inaccessible_dirs, INACCESSIBLE)) < 0) + return r; + + if (private_tmp) { + m->path = "/tmp"; + m->mode = PRIVATE_TMP; + m++; + + m->path = "/var/tmp"; + m->mode = PRIVATE_VAR_TMP; + m++; + } - if (private_tmp) { - m->path = "/tmp"; - m->mode = PRIVATE_TMP; - m++; + assert(mounts + n == m); - m->path = "/var/tmp"; - m->mode = PRIVATE_VAR_TMP; - m++; + qsort(mounts, n, sizeof(BindMount), mount_path_compare); + drop_duplicates(mounts, &n); } - assert(mounts + n == m); - - qsort(mounts, n, sizeof(BindMount), mount_path_compare); - drop_duplicates(mounts, &n); - /* Remount / as SLAVE so that nothing now mounted in the namespace shows up in the parent */ if (mount(NULL, "/", NULL, MS_SLAVE|MS_REC, NULL) < 0) diff --git a/src/journal/catalog.c b/src/journal/catalog.c index 7738d24..90ca008 100644 --- a/src/journal/catalog.c +++ b/src/journal/catalog.c @@ -399,7 +399,7 @@ int catalog_update(const char* database, const char* root, const char* const* di } assert(n == hashmap_size(h)); - qsort(items, n, sizeof(CatalogItem), catalog_compare_func); + qsort_safe(items, n, sizeof(CatalogItem), catalog_compare_func); r = write_catalog(database, h, sb, items, n); if (r < 0) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 81c344f..425e38a 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -1344,7 +1344,7 @@ int journal_file_append_entry(JournalFile *f, const dual_timestamp *ts, const st /* Order by the position on disk, in order to improve seek * times for rotating media. */ - qsort(items, n_iovec, sizeof(EntryItem), entry_item_cmp); + qsort_safe(items, n_iovec, sizeof(EntryItem), entry_item_cmp); r = journal_file_append_entry_internal(f, ts, xor_hash, items, n_iovec, seqnum, ret, offset); diff --git a/src/journal/journal-vacuum.c b/src/journal/journal-vacuum.c index 8d5effb..d4a1c6c 100644 --- a/src/journal/journal-vacuum.c +++ b/src/journal/journal-vacuum.c @@ -299,8 +299,7 @@ int journal_directory_vacuum( n_list ++; } - if (n_list > 0) - qsort(list, n_list, sizeof(struct vacuum_info), vacuum_compare); + qsort_safe(list, n_list, sizeof(struct vacuum_info), vacuum_compare); for (i = 0; i < n_list; i++) { struct statvfs ss; diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c index 9a2d255..0876ee6 100644 --- a/src/journal/journalctl.c +++ b/src/journal/journalctl.c @@ -755,7 +755,7 @@ static int get_relative_boot_id(sd_journal *j, sd_id128_t *boot_id, int relative sd_journal_flush_matches(j); } - qsort(all_ids, count, sizeof(boot_id_t), boot_id_cmp); + qsort_safe(all_ids, count, sizeof(boot_id_t), boot_id_cmp); if (sd_id128_equal(*boot_id, SD_ID128_NULL)) { if (relative > (int) count || relative <= -(int)count) diff --git a/src/libsystemd-bus/bus-match.c b/src/libsystemd-bus/bus-match.c index 1411167..916682a 100644 --- a/src/libsystemd-bus/bus-match.c +++ b/src/libsystemd-bus/bus-match.c @@ -768,7 +768,7 @@ int bus_match_parse( } /* Order the whole thing, so that we always generate the same tree */ - qsort(components, n_components, sizeof(struct bus_match_component), match_component_compare); + qsort_safe(components, n_components, sizeof(struct bus_match_component), match_component_compare); /* Check for duplicates */ for (i = 0; i+1 < n_components; i++) diff --git a/src/libudev/libudev-enumerate.c b/src/libudev/libudev-enumerate.c index 8146f27..e71d766 100644 --- a/src/libudev/libudev-enumerate.c +++ b/src/libudev/libudev-enumerate.c @@ -276,7 +276,7 @@ _public_ struct udev_list_entry *udev_enumerate_get_list_entry(struct udev_enume size_t move_later_prefix = 0; udev_list_cleanup(&udev_enumerate->devices_list); - qsort(udev_enumerate->devices, udev_enumerate->devices_cur, sizeof(struct syspath), syspath_cmp); + qsort_safe(udev_enumerate->devices, udev_enumerate->devices_cur, sizeof(struct syspath), syspath_cmp); max = udev_enumerate->devices_cur; for (i = 0; i < max; i++) { diff --git a/src/nss-myhostname/netlink.c b/src/nss-myhostname/netlink.c index b1ef912..47a41f5 100644 --- a/src/nss-myhostname/netlink.c +++ b/src/nss-myhostname/netlink.c @@ -197,7 +197,8 @@ finish: return r; } - qsort(list, n_list, sizeof(struct address), address_compare); + if (n_list) + qsort(list, n_list, sizeof(struct address), address_compare); *_list = list; *_n_list = n_list; diff --git a/src/readahead/readahead-collect.c b/src/readahead/readahead-collect.c index 32888ad..6b74866 100644 --- a/src/readahead/readahead-collect.c +++ b/src/readahead/readahead-collect.c @@ -536,8 +536,7 @@ done: HASHMAP_FOREACH_KEY(q, p, files, i) pack_file(pack, p, on_btrfs); } else { - struct item *ordered, *j; - unsigned k, n; + unsigned n; /* On rotating media, order things by the block * numbers */ @@ -545,25 +544,31 @@ done: log_debug("Ordering..."); n = hashmap_size(files); - if (!(ordered = new(struct item, n))) { - r = log_oom(); - goto finish; - } - - j = ordered; - HASHMAP_FOREACH_KEY(q, p, files, i) { - memcpy(j, q, sizeof(struct item)); - j++; - } + if (n) { + _cleanup_free_ struct item *ordered; + struct item *j; + unsigned k; + + ordered = new(struct item, n); + if (!ordered) { + r = log_oom(); + goto finish; + } - assert(ordered + n == j); + j = ordered; + HASHMAP_FOREACH_KEY(q, p, files, i) { + memcpy(j, q, sizeof(struct item)); + j++; + } - qsort(ordered, n, sizeof(struct item), qsort_compare); + assert(ordered + n == j); - for (k = 0; k < n; k++) - pack_file(pack, ordered[k].path, on_btrfs); + qsort(ordered, n, sizeof(struct item), qsort_compare); - free(ordered); + for (k = 0; k < n; k++) + pack_file(pack, ordered[k].path, on_btrfs); + } else + log_warning("No pack files"); } log_debug("Finalizing..."); diff --git a/src/shared/cgroup-show.c b/src/shared/cgroup-show.c index e971f36..cc44ab4 100644 --- a/src/shared/cgroup-show.c +++ b/src/shared/cgroup-show.c @@ -44,6 +44,8 @@ static void show_pid_array(int pids[], unsigned n_pids, const char *prefix, unsi unsigned i, m, pid_width; pid_t biggest = 0; + assert(n_pids > 0); + /* Filter duplicates */ m = 0; for (i = 0; i < n_pids; i++) { diff --git a/src/shared/conf-files.c b/src/shared/conf-files.c index 6d99739..ed4070c 100644 --- a/src/shared/conf-files.c +++ b/src/shared/conf-files.c @@ -127,7 +127,7 @@ static int conf_files_list_strv_internal(char ***strv, const char *suffix, const return -ENOMEM; } - qsort(files, hashmap_size(fh), sizeof(char *), base_cmp); + qsort_safe(files, hashmap_size(fh), sizeof(char *), base_cmp); *strv = files; hashmap_free(fh); diff --git a/src/shared/efivars.c b/src/shared/efivars.c index c015b16..f3eb6a6 100644 --- a/src/shared/efivars.c +++ b/src/shared/efivars.c @@ -384,8 +384,7 @@ int efi_get_boot_options(uint16_t **options) { list[count ++] = id; } - if (list) - qsort(list, count, sizeof(uint16_t), cmp_uint16); + qsort_safe(list, count, sizeof(uint16_t), cmp_uint16); *options = list; return count; diff --git a/src/shared/fileio.c b/src/shared/fileio.c index 603a1c7..733b320 100644 --- a/src/shared/fileio.c +++ b/src/shared/fileio.c @@ -662,6 +662,7 @@ int get_status_field(const char *filename, const char *pattern, char **field) { int r; assert(filename); + assert(pattern); assert(field); r = read_full_file(filename, &status, NULL); diff --git a/src/shared/util.h b/src/shared/util.h index 1b845b3..222abe0 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -764,3 +764,15 @@ bool id128_is_valid(const char *s) _pure_; void parse_user_at_host(char *arg, char **user, char **host); int split_pair(const char *s, const char *sep, char **l, char **r); + +/** + * Normal qsort requires base to be nonnull. Here were require + * that only if nmemb > 0. + */ +static inline void qsort_safe(void *base, size_t nmemb, size_t size, + int (*compar)(const void *, const void *)) { + if (nmemb) { + assert(base); + qsort(base, nmemb, size, compar); + } +} diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index d75281f..036828b 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -471,7 +471,7 @@ static int list_units(DBusConnection *bus, char **args) { if (r < 0) return r; - qsort(unit_infos, c, sizeof(struct unit_info), compare_unit_info); + qsort_safe(unit_infos, c, sizeof(struct unit_info), compare_unit_info); output_units_list(unit_infos, c); @@ -733,8 +733,8 @@ static int list_sockets(DBusConnection *bus, char **args) { listen = triggered = NULL; /* avoid cleanup */ } - qsort(socket_infos, cs, sizeof(struct socket_info), - (__compar_fn_t) socket_info_compare); + qsort_safe(socket_infos, cs, sizeof(struct socket_info), + (__compar_fn_t) socket_info_compare); output_sockets_list(socket_infos, cs); @@ -1108,7 +1108,7 @@ static int list_dependencies_one(DBusConnection *bus, const char *name, int leve if (r < 0) return r; - qsort(deps, strv_length(deps), sizeof (char*), list_dependencies_compare); + qsort_safe(deps, strv_length(deps), sizeof (char*), list_dependencies_compare); STRV_FOREACH(c, deps) { if (strv_contains(u, *c)) { @@ -3532,7 +3532,7 @@ static int show_all(const char* verb, if (r < 0) return r; - qsort(unit_infos, c, sizeof(struct unit_info), compare_unit_info); + qsort_safe(unit_infos, c, sizeof(struct unit_info), compare_unit_info); for (u = unit_infos; u < unit_infos + c; u++) { _cleanup_free_ char *p = NULL; -- 1.8.4.652.g0d6e0ce