[LEDE-DEV] [PATCH 2/2] fstools: Fix some errors detected by cppcheck
Rosen Penev
rosenp at gmail.com
Sun Dec 10 12:59:09 PST 2017
Reposting since gmail sucks:
On Sun, Dec 10, 2017 at 10:17 AM, Arjen de Korte
<arjen+lede at de-korte.org> wrote:
> 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.
>
will add a check
>> @@ -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?
>
null pointer dereference. The check for pr being null is after the declarations.
>> 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);
> }
>
cppcheck no longer complains after this change and removing the
previous device_free
>> 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.
>
>
this change just lowers compiled size by 16 bytes. not sure why.
the goal was to close as soon as possible to avoid adding an extra
fclose call. Saves a few bytes.
this is optimized away. I just saw it as a way to make the code a
little clearer. I can revert if needed.
>> 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?
>
consistency update with the other memset. no functional difference.
On Sun, Dec 10, 2017 at 11:00 AM, Rosen Penev <rosenp at gmail.com> wrote:
> On Sun, Dec 10, 2017 at 10:17 AM, Arjen de Korte
> <arjen+lede at de-korte.org> wrote:
>> 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.
>>
> will add a check
>>> @@ -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?
>>
> null pointer dereference. The check for pr being null is after the declarations.
>>> 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);
>> }
>>
> cppcheck no longer complains after this change and removing the
> previous device_free
>
>>> 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.
>>
>>
> this change just lowers compiled size by 16 bytes. not sure why.
>>> 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.
>>
> the goal was to close as soon as possible to avoid adding an extra
> fclose call. Saves a few bytes.
>>
>>> 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.
>>
> this is optimized away. I just saw it as a way to make the code a
> little clearer. I can revert if needed.
>>> 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?
>>
> consistency update with the other memset. no functional difference.
>>> 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.
>>
>>
>> _______________________________________________
>> Lede-dev mailing list
>> Lede-dev at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/lede-dev
More information about the Lede-dev
mailing list