Re: fsck_msdos: memleak merge with NetBSD
On Mon, Jun 30, 2014 at 21:26, Tobias Stoeckmann wrote: Hi, this diff merges NetBSD's revision 1.20 into our tree: There are some memory leaks in resetDosDirSection. This is not a simple merge, I have added some things: - rootDir was not properly freed in NetBSD's commit (actually it's put into a free dir entry queue) - also free memory if root directory checks fail - I use goto's instead of rewriting the code every single time +fail: + freeDosDirEntry(rootDir); + rootDir = NULL; +fail_root: + free(delbuf); + delbuf = NULL; +fail_delbuf: + free(buffer); + buffer = NULL; + return (FSFATAL); I think it's safe to just have one goto label? free(NULL) is fine. a small preference, but it seems less error prone when there's one cleanup section that does what's necessary, as opposed to the jumping code needing to determine where to jump each time.
Re: fsck_msdos: memleak merge with NetBSD
On Tue, Jul 08, 2014 at 03:01:58PM -0400, Ted Unangst wrote:
I think it's safe to just have one goto label? free(NULL) is fine.
That's true. If we go into that direction, we can also use the cleanup
function that would be done at the end of directory processing.
Also put two variables static that are not used anywhere else, easier
to verify that there are no side-effects.
This version looks definitely simpler.
Tobias
Index: dir.c
===
RCS file: /cvs/src/sbin/fsck_msdos/dir.c,v
retrieving revision 1.23
diff -u -p -r1.23 dir.c
--- dir.c 18 Jun 2014 17:29:07 - 1.23
+++ dir.c 8 Jul 2014 19:29:54 -
@@ -147,7 +147,7 @@ freeDirTodo(struct dirTodoNode *dt)
/*
* The stack of unread directories
*/
-struct dirTodoNode *pendingDirectories = NULL;
+static struct dirTodoNode *pendingDirectories = NULL;
/*
* Return the full pathname for a directory entry.
@@ -203,7 +203,7 @@ static char longName[DOSLONGNAMELEN] =
static u_char *buffer = NULL;
static u_char *delbuf = NULL;
-struct dosDirEntry *rootDir;
+static struct dosDirEntry *rootDir;
static struct dosDirEntry *lostDir;
/*
@@ -223,14 +223,14 @@ resetDosDirSection(struct bootblock *boo
|| !(delbuf = malloc(b2))
|| !(rootDir = newDosDirEntry())) {
xperror(No space for directory);
- return (FSFATAL);
+ goto fail;
}
(void)memset(rootDir, 0, sizeof *rootDir);
if (boot-flags FAT32) {
if (boot-RootCl CLUST_FIRST || boot-RootCl =
boot-NumClusters) {
pfatal(Root directory starts with cluster out of
range(%u)\n,
boot-RootCl);
- return (FSFATAL);
+ goto fail;
}
cl = fat[boot-RootCl].next;
if (cl CLUST_FIRST
@@ -243,7 +243,7 @@ resetDosDirSection(struct bootblock *boo
rsrvdcltype(cl));
else {
pfatal(Root directory doesn't start a cluster
chain\n);
- return (FSFATAL);
+ goto fail;
}
if (ask(1, Fix)) {
fat[boot-RootCl].next = CLUST_FREE;
@@ -257,6 +257,9 @@ resetDosDirSection(struct bootblock *boo
}
return (ret);
+fail:
+ finishDosDirSection();
+ return (FSFATAL);
}
/*
Index: ext.h
===
RCS file: /cvs/src/sbin/fsck_msdos/ext.h,v
retrieving revision 1.11
diff -u -p -r1.11 ext.h
--- ext.h 16 Jun 2014 18:33:33 - 1.11
+++ ext.h 8 Jul 2014 19:29:54 -
@@ -46,8 +46,6 @@ extern int rdonly;/* device is opened r
extern struct disklabel lab;
-extern struct dosDirEntry *rootDir;
-
/*
* function declarations
*/
fsck_msdos: memleak merge with NetBSD
Hi,
this diff merges NetBSD's revision 1.20 into our tree: There are
some memory leaks in resetDosDirSection.
This is not a simple merge, I have added some things:
- rootDir was not properly freed in NetBSD's commit
(actually it's put into a free dir entry queue)
- also free memory if root directory checks fail
- I use goto's instead of rewriting the code every single time
- kept our local styles (like xperror vs perr)
- after free, set values to NULL; better safe than sorry and also
in sync with other functions
Tobias
Index: dir.c
===
RCS file: /cvs/src/sbin/fsck_msdos/dir.c,v
retrieving revision 1.23
diff -u -p -r1.23 dir.c
--- dir.c 18 Jun 2014 17:29:07 - 1.23
+++ dir.c 30 Jun 2014 19:19:26 -
@@ -219,18 +219,27 @@ resetDosDirSection(struct bootblock *boo
b1 = boot-RootDirEnts * 32;
b2 = boot-SecPerClust * boot-BytesPerSec;
- if (!(buffer = malloc(b1 b2 ? b1 : b2))
- || !(delbuf = malloc(b2))
- || !(rootDir = newDosDirEntry())) {
+ if ((buffer = malloc(b1 b2 ? b1 : b2)) == NULL) {
xperror(No space for directory);
return (FSFATAL);
}
+
+ if ((delbuf = malloc(b2)) == NULL) {
+ xperror(No space for directory delbuf);
+ goto fail_delbuf;
+ }
+
+ if ((rootDir = newDosDirEntry()) == NULL) {
+ xperror(No space for directory entry);
+ goto fail_root;
+ }
+
(void)memset(rootDir, 0, sizeof *rootDir);
if (boot-flags FAT32) {
if (boot-RootCl CLUST_FIRST || boot-RootCl =
boot-NumClusters) {
pfatal(Root directory starts with cluster out of
range(%u)\n,
boot-RootCl);
- return (FSFATAL);
+ goto fail;
}
cl = fat[boot-RootCl].next;
if (cl CLUST_FIRST
@@ -243,7 +252,7 @@ resetDosDirSection(struct bootblock *boo
rsrvdcltype(cl));
else {
pfatal(Root directory doesn't start a cluster
chain\n);
- return (FSFATAL);
+ goto fail;
}
if (ask(1, Fix)) {
fat[boot-RootCl].next = CLUST_FREE;
@@ -257,6 +266,16 @@ resetDosDirSection(struct bootblock *boo
}
return (ret);
+fail:
+ freeDosDirEntry(rootDir);
+ rootDir = NULL;
+fail_root:
+ free(delbuf);
+ delbuf = NULL;
+fail_delbuf:
+ free(buffer);
+ buffer = NULL;
+ return (FSFATAL);
}
/*
