Citeren Rosen Penev <ros...@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 <ros...@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.
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
