| Message ID | 1512983081-22539-1-git-send-email-rosenp@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [LEDE-DEV] fstools: Fix some errors detected by cppcheck | expand |
On 11 December 2017 at 10:04, Rosen Penev <rosenp@gmail.com> wrote:
> Mainly plugging memory leaks. Size reduction as well. The calloc change accounts for 272 bytes on this machine for some reason...
Please state the exact errors found by cppcheck so we don't have to
guess what it found.
Regards
Jonas
Citeren Jonas Gorski <jonas.gorski@gmail.com>: > On 11 December 2017 at 10:04, Rosen Penev <rosenp@gmail.com> wrote: >> Mainly plugging memory leaks. Size reduction as well. The calloc >> change accounts for 272 bytes on this machine for some reason... > > Please state the exact errors found by cppcheck so we don't have to > guess what it found. I tried to replicate what Rosen did, but in my case cppcheck came up with only two errors: [libfstools/rootdisk.c:175]: (error) Resource leak: f [libfstools/ubi.c:72]: (error) Resource leak: f These look like actual leaks, so this *may* be worth fixing. It could also be the conditions never occur (so these never leak anything). The proposed changes for these two errors look sane though. [block.c:1087] -> [block.c:1091]: (warning) Either the condition '!pr' is redundant or there is possible null pointer dereference: pr. This one has merit too. There is no point in doing a NULL pointer check after using that pointer a few lines before. [blockd.c:119]: (warning) Possible null pointer dereference: device [blockd.c:127]: (warning) Possible null pointer dereference: device [blockd.c:130]: (warning) Possible null pointer dereference: device I'm not so sure about these warnings. It doesn't seem possible that *both* device_n and device_o are NULL, which would be required to actually cause a NULL pointer dereference here. This might be fixed cleaner by changing 211 } else { to 211 } else if (device_o) { The remainder of the warnings might be 'fixed' to shut-up cppcheck, but don't actually change anything.
On Mon, Dec 11, 2017 at 1:22 PM, Arjen de Korte <arjen+lede@de-korte.org> wrote: > Citeren Jonas Gorski <jonas.gorski@gmail.com>: > >> On 11 December 2017 at 10:04, Rosen Penev <rosenp@gmail.com> wrote: >>> >>> Mainly plugging memory leaks. Size reduction as well. The calloc change >>> accounts for 272 bytes on this machine for some reason... >> >> >> Please state the exact errors found by cppcheck so we don't have to >> guess what it found. > I used "cppcheck --enable=all --inconclusive --force . 2>err.txt" to find them. Then I just filtered. cat err.txt | grep -v never | grep -v reduced output: $ cat err.txt | grep -v never | grep -v reduced [block.c:1087] -> [block.c:1091]: (warning) Either the condition '!pr' is redundant or there is possible null pointer dereference: pr. [block.c:1244] -> [block.c:1246]: (style) Variable 'err' is reassigned a value before the old one has been used. [block.c:1257] -> [block.c:1259]: (style) Variable 'err' is reassigned a value before the old one has been used. [block.c:1330] -> [block.c:1334]: (style) Variable 'err' is reassigned a value before the old one has been used. [blockd.c:119]: (warning) Possible null pointer dereference: device [blockd.c:122]: (warning) Possible null pointer dereference: device [blockd.c:127]: (warning) Possible null pointer dereference: device [blockd.c:130]: (warning) Possible null pointer dereference: device [libblkid-tiny\libblkid-tiny.c:188]: (style) Label 'int' is not used. [libfstools\overlay.c:290]: (warning) Obsolete function 'alloca' called. In C99 and later it is recommended to use a variable length array instead. [libfstools\overlay.c:314]: (warning) Obsolete function 'alloca' called. In C99 and later it is recommended to use a variable length array instead. [libfstools\rootdisk.c:175]: (error) Resource leak: f [libfstools\ubi.c:106]: (warning) %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [libfstools\ubi.c:106]: (warning) %u in format string (no. 3) requires 'unsigned int' but the argument type is 'signed int'. [libfstools\ubi.c:106]: (warning) %u in format string (no. 4) requires 'unsigned int' but the argument type is 'signed int'. [libfstools\ubi.c:109]: (warning) %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [libfstools\ubi.c:109]: (warning) %u in format string (no. 2) requires 'unsigned int' but the argument type is 'signed int'. [libfstools\ubi.c:132]: (warning) %u in format string (no. 2) requires 'unsigned int' but the argument type is 'int'. [libfstools\ubi.c:132]: (warning) %u in format string (no. 3) requires 'unsigned int' but the argument type is 'int'. [libfstools\ubi.c:132]: (warning) %u in format string (no. 4) requires 'unsigned int' but the argument type is 'int'. [libfstools\ubi.c:135]: (warning) %u in format string (no. 1) requires 'unsigned int' but the argument type is 'int'. [libfstools\ubi.c:135]: (warning) %u in format string (no. 2) requires 'unsigned int' but the argument type is 'int'. [libfstools\ubi.c:72]: (error) Resource leak: f [libubi\libubi-tiny.c:61]: (portability) 'buf' is of type 'const void *'. When using void pointers in calculations, the behaviour is undefined. [libubi\libubi.c:858]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1026]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1039]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1067]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1095]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1122]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1138]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:1148]: (warning) Redundant assignment of 'desc' to itself. [libubi\libubi.c:985]: (warning) sscanf() without field width limits can crash with huge input data. [libubi\libubi.c:1202]: (warning) sscanf() without field width limits can crash with huge input data. [libubi\ubiutils-common.c:122]: (style) Redundant condition: If 'bytes > 1024', the comparison 'bytes != 0' is always true. (information) Cppcheck cannot find all the include files (use --check-config for details) I fixed what I believed was useful, not every warning. > > I tried to replicate what Rosen did, but in my case cppcheck came up with > only two errors: > > [libfstools/rootdisk.c:175]: (error) Resource leak: f > [libfstools/ubi.c:72]: (error) Resource leak: f > > These look like actual leaks, so this *may* be worth fixing. It could also > be the conditions never occur (so these never leak anything). The proposed > changes for these two errors look sane though. > > [block.c:1087] -> [block.c:1091]: (warning) Either the condition '!pr' > is redundant or there is possible null pointer dereference: pr. > > This one has merit too. There is no point in doing a NULL pointer check > after using that pointer a few lines before. > > [blockd.c:119]: (warning) Possible null pointer dereference: device > [blockd.c:127]: (warning) Possible null pointer dereference: device > [blockd.c:130]: (warning) Possible null pointer dereference: device > > I'm not so sure about these warnings. It doesn't seem possible that *both* > device_n and device_o are NULL, which would be required to actually cause a > NULL pointer dereference here. This might be fixed cleaner by changing > > 211 } else { > > to > > 211 } else if (device_o) { > > The remainder of the warnings might be 'fixed' to shut-up cppcheck, but > don't actually change anything. > For v2 of this patch, I added some stuff that reduces total compiled size (changed functions to void since return value is not used). If needed, I can split it up into two to deal with only cppcheck in the first, and size optimizations in the second. PS: No I have no idea why size gets reduced like this. The most ridiculous example from a different project is where I changed a function's return type from int to bool. GCC output went from 61440 to 57560 while clang was unaffected. > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
diff --git a/block.c b/block.c index ab130b9..851cd05 100644 --- a/block.c +++ b/block.c @@ -254,7 +254,7 @@ static void parse_mount_options(struct mount *m, char *optstr) free(optstr); } -static int mount_add(struct uci_section *s) +static void mount_add(struct uci_section *s) { struct blob_attr *tb[__MOUNT_MAX] = { 0 }; struct mount *m; @@ -264,10 +264,10 @@ static int mount_add(struct uci_section *s) blobmsg_parse(mount_policy, __MOUNT_MAX, tb, blob_data(b.head), blob_len(b.head)); if (!tb[MOUNT_LABEL] && !tb[MOUNT_UUID] && !tb[MOUNT_DEVICE]) - return -1; + return; if (tb[MOUNT_ENABLE] && !blobmsg_get_u32(tb[MOUNT_ENABLE])) - return -1; + return; m = malloc(sizeof(struct mount)); m->type = TYPE_MOUNT; @@ -291,7 +291,7 @@ static int mount_add(struct uci_section *s) ULOG_WARN("ignoring mount section %s due to invalid target '%s'\n", s->e.name, m->target); free(m); - return -1; + return; } if (m->uuid) @@ -300,11 +300,9 @@ static int mount_add(struct uci_section *s) vlist_add(&mounts, &m->node, m->label); else if (m->device) vlist_add(&mounts, &m->node, m->device); - - return 0; } -static int swap_add(struct uci_section *s) +static void swap_add(struct uci_section *s) { struct blob_attr *tb[__SWAP_MAX] = { 0 }; struct mount *m; @@ -314,10 +312,11 @@ static int swap_add(struct uci_section *s) blobmsg_parse(swap_policy, __SWAP_MAX, tb, blob_data(b.head), blob_len(b.head)); if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE]) - return -1; + return; - m = malloc(sizeof(struct mount)); - memset(m, 0, sizeof(struct mount)); + m = calloc(1, sizeof(struct mount)); + if (!m) + return; m->type = TYPE_SWAP; m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]); m->label = blobmsg_get_strdup(tb[SWAP_LABEL]); @@ -339,11 +338,9 @@ static int swap_add(struct uci_section *s) else if (m->device) vlist_add(&mounts, &m->node, m->device); } - - return 0; } -static int global_add(struct uci_section *s) +static void global_add(struct uci_section *s) { struct blob_attr *tb[__CFG_MAX] = { 0 }; @@ -366,8 +363,6 @@ static int global_add(struct uci_section *s) if ((tb[CFG_CHECK_FS]) && blobmsg_get_u32(tb[CFG_CHECK_FS])) check_fs = 1; - - return 0; } static struct mount* find_swap(const char *uuid, const char *label, const char *device) @@ -497,14 +492,14 @@ static struct probe_info* _probe_path(char *path) return probe_path(path); } -static int _cache_load(const char *path) +static void _cache_load(const char *path) { int gl_flags = GLOB_NOESCAPE | GLOB_MARK; int j; glob_t gl; if (glob(path, gl_flags, NULL, &gl) < 0) - return -1; + return; for (j = 0; j < gl.gl_pathc; j++) { struct probe_info *pr = _probe_path(gl.gl_pathv[j]); @@ -513,8 +508,6 @@ static int _cache_load(const char *path) } globfree(&gl); - - return 0; } static void cache_load(int mtd) @@ -1084,7 +1077,7 @@ static int mount_device(struct probe_info *pr, int type) static int umount_device(struct probe_info *pr) { struct mount *m; - char *device = basename(pr->dev); + char *device; char *mp; int err; @@ -1098,6 +1091,7 @@ static int umount_device(struct probe_info *pr) if (!mp) return -1; + device = basename(pr->dev); m = find_block(pr->uuid, pr->label, device, NULL); if (m && m->extroot) return -1; diff --git a/blockd.c b/blockd.c index 3af5390..f7a4dec 100644 --- a/blockd.c +++ b/blockd.c @@ -115,6 +115,9 @@ device_free(struct device *device) char *target = NULL; char *path = NULL, _path[64], *mp; + if (!device) + return; + blobmsg_parse(mount_policy, __MOUNT_MAX, data, blob_data(device->msg), blob_len(device->msg)); diff --git a/libfstools/overlay.c b/libfstools/overlay.c index 7ada5ff..d7a55e6 100644 --- a/libfstools/overlay.c +++ b/libfstools/overlay.c @@ -284,7 +284,7 @@ enum fs_state fs_state_get(const char *dir) { char *path; char valstr[16]; - uint32_t val; + int val; ssize_t len; path = alloca(strlen(dir) + 1 + sizeof("/.fs_state")); diff --git a/libfstools/rootdisk.c b/libfstools/rootdisk.c index dd00c1b..2bb16d4 100644 --- a/libfstools/rootdisk.c +++ b/libfstools/rootdisk.c @@ -171,8 +171,10 @@ static int rootdisk_volume_identify(struct volume *v) fseeko(f, p->offset + 0x400, SEEK_SET); n = fread(&magic, sizeof(magic), 1, f); - if (n != 1) + if (n != 1) { + fclose(f); return -1; + } if (magic == cpu_to_le32(0xF2F52010)) ret = FS_F2FS; @@ -180,13 +182,12 @@ static int rootdisk_volume_identify(struct volume *v) magic = 0; fseeko(f, p->offset + 0x438, SEEK_SET); n = fread(&magic, sizeof(magic), 1, f); + fclose(f); if (n != 1) return -1; if ((le32_to_cpu(magic) & 0xffff) == 0xef53) ret = FS_EXT4; - fclose(f); - return ret; } diff --git a/libfstools/snapshot.c b/libfstools/snapshot.c index 4870cf7..58bed96 100644 --- a/libfstools/snapshot.c +++ b/libfstools/snapshot.c @@ -203,16 +203,19 @@ snapshot_read_file(struct volume *v, int block, char *file, uint32_t type) if (hdr.length < len) len = hdr.length; - if (volume_read(v, buffer, offset, len)) + if (volume_read(v, buffer, offset, len)) { + close(out); return -1; - if (write(out, buffer, len) != len) + } + + int w = write(out, buffer, len); + close(out); + if (w != len) return -1; offset += len; hdr.length -= len; } - close(out); - if (verify_file_hash(file, hdr.md5)) { ULOG_ERR("md5 verification failed\n"); unlink(file); diff --git a/libfstools/ubi.c b/libfstools/ubi.c index f9d6e0a..678b8bf 100644 --- a/libfstools/ubi.c +++ b/libfstools/ubi.c @@ -60,6 +60,7 @@ static char FILE *f; char fname[BUFLEN]; char buf[BUFLEN]; + char *s; int i; snprintf(fname, sizeof(fname), "%s/%s", dirname, filename); @@ -68,10 +69,10 @@ static char if (!f) return NULL; - if (fgets(buf, sizeof(buf), f) == NULL) - return NULL; - + s = fgets(buf, sizeof(buf), f); fclose(f); + if (s == NULL) + return NULL; /* make sure the string is \0 terminated */ buf[sizeof(buf) - 1] = '\0'; @@ -84,17 +85,17 @@ static char return strdup(buf); } -static unsigned int +static bool test_open(char *filename) { FILE *f; f = fopen(filename, "r"); if (!f) - return 0; + return false; fclose(f); - return 1; + return true; } static int ubi_volume_init(struct volume *v) diff --git a/libubi/libubi.c b/libubi/libubi.c index 3328ac8..3082a10 100644 --- a/libubi/libubi.c +++ b/libubi/libubi.c @@ -427,6 +427,7 @@ static int vol_node2nums(struct libubi *lib, const char *node, int *dev_num, return -1; } + close(fd); *dev_num = i; *vol_id = minor - 1; errno = 0; @@ -1021,9 +1022,8 @@ int ubi_mkvol(libubi_t desc, const char *node, struct ubi_mkvol_request *req) struct ubi_mkvol_req r; size_t n; - memset(&r, 0, sizeof(struct ubi_mkvol_req)); - desc = desc; + memset(&r, 0, sizeof(struct ubi_mkvol_req)); r.vol_id = req->vol_id; r.alignment = req->alignment; r.bytes = req->bytes; diff --git a/libubi/ubiutils-common.c b/libubi/ubiutils-common.c index 2271927..8ea2815 100644 --- a/libubi/ubiutils-common.c +++ b/libubi/ubiutils-common.c @@ -119,7 +119,7 @@ void ubiutils_print_bytes(long long bytes, int bracket) printf("%s%.1f GiB", p, (double)bytes / (1024 * 1024 * 1024)); else if (bytes > 1024 * 1024) printf("%s%.1f MiB", p, (double)bytes / (1024 * 1024)); - else if (bytes > 1024 && bytes != 0) + else if (bytes > 1024) printf("%s%.1f KiB", p, (double)bytes / 1024); else return;
Mainly plugging memory leaks. Size reduction as well. The calloc change accounts for 272 bytes on this machine for some reason... Signed-off-by: Rosen Penev <rosenp@gmail.com> v2: Add calloc fail check + turn some functions to void where it saves size. Total savings = 184 bytes after everything. --- block.c | 34 ++++++++++++++-------------------- blockd.c | 3 +++ libfstools/overlay.c | 2 +- libfstools/rootdisk.c | 7 ++++--- libfstools/snapshot.c | 11 +++++++---- libfstools/ubi.c | 13 +++++++------ libubi/libubi.c | 4 ++-- libubi/ubiutils-common.c | 2 +- 8 files changed, 39 insertions(+), 37 deletions(-)