Skip to contentSkip to conversation

fs/fat: fix fatbuf leak

PATCH In review Rob Clark 4 participants 5 messages Sep 12, 2017 – Sep 16, 2017 VFAT/FAT/msdos filesystem

lore
Share & tools
lore

Rob Clark

author
#0 lore raw

PATCH 1/1 fs/fat: fix fatbuf leak

A new fatbuf was allocated by get_fs_info() (called by fat_itr_root()), but not freed, resulting in eventually running out of memory. Spotted by running 'ls -r' in a large FAT filesystem from Shell.efi.

fatbuf is mainly used to cache FAT entry lookups (get_fatent()).. possibly once fat_write.c it can move into the iterator to simplify this.

---
I can squash this back in to the earlier readdir patches and resend them if that is preferred.

 fs/fat/fat.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
fs/fat/fat.c +16 -7
index f5f74c12ff..f0284398b4 100644
@@ -1042,6 +1042,7 @@ int fat_exists(const char *filename)
 		return 0;
 
 	ret = fat_itr_resolve(itr, filename, TYPE_ANY);
+	free(fsdata.fatbuf);
 	return ret == 0;
 }
 
@@ -1061,17 +1062,19 @@ int fat_size(const char *filename, loff_t *size)
 		 * Directories don't have size, but fs_size() is not
 		 * expected to fail if passed a directory path:
 		 */
+		free(fsdata.fatbuf);
 		fat_itr_root(itr, &fsdata);
 		if (!fat_itr_resolve(itr, filename, TYPE_DIR)) {
 			*size = 0;
-			return 0;
+			ret = 0;
 		}
-		return ret;
+		goto out;
 	}
 
 	*size = FAT2CPU32(itr->dent->size);
-
-	return 0;
+out:
+	free(fsdata.fatbuf);
+	return ret;
 }
 
 int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
@@ -1087,10 +1090,14 @@ int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
 	ret = fat_itr_resolve(itr, filename, TYPE_FILE);
 	if (ret)
-		return ret;
+		goto out;
 
 	printf("reading %s\n", filename);
-	return get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
+	ret = get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
+
+out:
+	free(fsdata.fatbuf);
+	return ret;
 }
 
 int file_fat_read(const char *filename, void *buffer, int maxsize)
@@ -1126,7 +1133,7 @@ typedef struct {
 
 int fat_opendir(const char *filename, struct fs_dir_stream **dirsp)
 {
-	fat_dir *dir = malloc(sizeof(*dir));
+	fat_dir *dir = calloc(1, sizeof(*dir));
 	int ret;
 
 	if (!dir)
@@ -1144,6 +1151,7 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp)
 	return 0;
 
 fail:
+	free(dir->fsdata.fatbuf);
 	free(dir);
 	return ret;
 }
@@ -1174,6 +1182,7 @@ int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp)
 void fat_closedir(struct fs_dir_stream *dirs)
 {
 	fat_dir *dir = (fat_dir *)dirs;
+	free(dir->fsdata.fatbuf);
 	free(dir);
 }
 
Signature · 1 line
2.13.5
#1 lore raw

PATCH 1/1 fs/fat: fix fatbuf leak

Quoted 8 lines · Rob Clark

A new fatbuf was allocated by get_fs_info() (called by fat_itr_root()), but not freed, resulting in eventually running out of memory. Spotted by running 'ls -r' in a large FAT filesystem from Shell.efi.

fatbuf is mainly used to cache FAT entry lookups (get_fatent()).. possibly once fat_write.c it can move into the iterator to simplify this.

Quoted 7 lines

---
I can squash this back in to the earlier readdir patches and resend them if that is preferred.

 fs/fat/fat.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
#2 lore raw

PATCH 1/1 fs/fat: fix fatbuf leak

In reply to Rob Clark
Quoted 88 lines · Rob Clark

A new fatbuf was allocated by get_fs_info() (called by fat_itr_root()), but not freed, resulting in eventually running out of memory. Spotted by running 'ls -r' in a large FAT filesystem from Shell.efi.

fatbuf is mainly used to cache FAT entry lookups (get_fatent()).. possibly once fat_write.c it can move into the iterator to simplify this.

---
I can squash this back in to the earlier readdir patches and resend them if that is preferred.

  fs/fat/fat.c | 23 ++++++++++++++++-------
  1 file changed, 16 insertions(+), 7 deletions(-)
fs/fat/fat.c +15 -7
index f5f74c12ff..f0284398b4 100644
@@ -1042,6 +1042,7 @@ int fat_exists(const char *filename)
  		return 0;
 
  	ret = fat_itr_resolve(itr, filename, TYPE_ANY);
+	free(fsdata.fatbuf);
  	return ret == 0;
  }
 
@@ -1061,17 +1062,19 @@ int fat_size(const char *filename, loff_t *size)
  		 * Directories don't have size, but fs_size() is not
  		 * expected to fail if passed a directory path:
  		 */
+		free(fsdata.fatbuf);
  		fat_itr_root(itr, &fsdata);
  		if (!fat_itr_resolve(itr, filename, TYPE_DIR)) {
  			*size = 0;
-			return 0;
+			ret = 0;
  		}
-		return ret;
+		goto out;
  	}
 
  	*size = FAT2CPU32(itr->dent->size);
-
-	return 0;
+out:
+	free(fsdata.fatbuf);
+	return ret;
  }
 
  int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
@@ -1087,10 +1090,14 @@ int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
 
  	ret = fat_itr_resolve(itr, filename, TYPE_FILE);
  	if (ret)
-		return ret;
+		goto out;
 
  	printf("reading %s\n", filename);
-	return get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
+	ret = get_contents(&fsdata, itr->dent, pos, buffer, maxsize, actread);
+
+out:
+	free(fsdata.fatbuf);
+	return ret;
  }
 
  int file_fat_read(const char *filename, void *buffer, int maxsize)
@@ -1126,7 +1133,7 @@ typedef struct {
 
  int fat_opendir(const char *filename, struct fs_dir_stream **dirsp)
  {
-	fat_dir *dir = malloc(sizeof(*dir));
+	fat_dir *dir = calloc(1, sizeof(*dir));
  	int ret;
 
  	if (!dir)
@@ -1144,6 +1151,7 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp)
  	return 0;
 
  fail:
+	free(dir->fsdata.fatbuf);
  	free(dir);
  	return ret;
  }

fs/fat/fat.c int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp) · line 1188

Quoted 7 lines
fs/fat/fat.c +1 -0
@@ -1174,6 +1182,7 @@ int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp)
  void fat_closedir(struct fs_dir_stream *dirs)
  {
  	fat_dir *dir = (fat_dir *)dirs;
+	free(dir->fsdata.fatbuf);
  	free(dir);
  }
 
Signature · 9 lines
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
#3 lore raw

PATCH 1/1 fs/fat: fix fatbuf leak

In reply to Rob Clark
Quoted 4 lines · Rob Clark

A new fatbuf was allocated by get_fs_info() (called by fat_itr_root()), but not freed, resulting in eventually running out of memory. Spotted by running 'ls -r' in a large FAT filesystem from Shell.efi.

Quoted 8 lines

fatbuf is mainly used to cache FAT entry lookups (get_fatent()).. possibly once fat_write.c it can move into the iterator to simplify this.

---
I can squash this back in to the earlier readdir patches and resend them if that is preferred.

Separate is fine, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170913/4c7383c7/attachment.sig>
#4 lore raw
In reply to Rob Clark
Quoted 11 lines · Rob Clark

A new fatbuf was allocated by get_fs_info() (called by fat_itr_root()), but not freed, resulting in eventually running out of memory. Spotted by running 'ls -r' in a large FAT filesystem from Shell.efi.

fatbuf is mainly used to cache FAT entry lookups (get_fatent()).. possibly once fat_write.c it can move into the iterator to simplify this.

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170915/6e5fa42e/attachment.sig>