[LEDE-DEV] [PATCH 2/2] fstools: Fix some errors detected by cppcheck
Arjen de Korte
arjen+lede at de-korte.org
Sun Dec 10 10:17:07 PST 2017
Citeren Rosen Penev <rosenp at gmail.com>:
> Mainly plugging memory leaks. Size reduction as well. The calloc
> change accounts for 272 bytes on this machine for some reason...
Comments inline.
> Signed-off-by: Rosen Penev <rosenp at gmail.com>
> ---
> block.c | 6 +++---
> 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, 28 insertions(+), 20 deletions(-)
>
> diff --git a/block.c b/block.c
> index ab130b9..6117701 100644
> --- a/block.c
> +++ b/block.c
> @@ -316,8 +316,7 @@ static int swap_add(struct uci_section *s)
> if (!tb[SWAP_UUID] && !tb[SWAP_LABEL] && !tb[SWAP_DEVICE])
> return -1;
>
> - m = malloc(sizeof(struct mount));
> - memset(m, 0, sizeof(struct mount));
> + m = calloc(1, sizeof(struct mount));
> m->type = TYPE_SWAP;
> m->uuid = blobmsg_get_strdup(tb[SWAP_UUID]);
> m->label = blobmsg_get_strdup(tb[SWAP_LABEL]);
This fails to address the real issue here, there is no check for a
NULL pointer after allocating memory.
> @@ -1084,7 +1083,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 +1097,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;
What exactly is the above change supposed to fix?
> 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));
>
I would rather fix this in the devices_update_cb() function instead.
Only in line 212 there is a change the device_free() function is
called will a NULL pointer, but a couple of lines down, there is
already a check for a NULL pointer. One might fix this in one go by
using
if (device_o) {
device_free(device_o);
free(device_o);
}
> 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"));
This just silences a warning, without fixing the (potential)
underlying problem. If the result of the atoi() somehow is not a
member of the enum fs_state (which is guaranteed > 0 numerically), the
function will return a non-member either way.
> 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;
> }
>
This actually makes sense to prevent leaking file pointers.
> 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);
Same here, this makes sense to do.
> 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';
Same here, makes sense.
> @@ -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)
I don't see a lot of merit here. Since this function is static, I have
to see the first compiler that doesn't optimize this away. I would be
surprised if this would produce different binaries.
> 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;
Makes sense.
> @@ -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;
What's the rationale here?
> 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;
Nice catch.
More information about the Lede-dev
mailing list