On Tue, Sep 24, 2013 at 08:16:47PM -0400, Kevin O'Connor wrote:
On Wed, Sep 25, 2013 at 03:11:20AM +0300, Michael S. Tsirkin wrote:
On Tue, Sep 24, 2013 at 07:55:52PM -0400, Kevin O'Connor wrote:
In all real-world situations we'll have megabytes of ram for "tmp" allocations. It's not a problem to over allocate here.
Right. But the code doesn't get simpler in fact - unless you think it's important to drop the loop counting nfiles?
I was pointing out the loop, not the malloc call. As before, it's a nitpick.
-Kevin
Ah, I see what you mean then. something like this on top will do the trick. Yes, it's simpler.
diff --git a/src/fw/romfile_loader.c b/src/fw/romfile_loader.c index c03fe48..2196a84 100644 --- a/src/fw/romfile_loader.c +++ b/src/fw/romfile_loader.c @@ -130,30 +130,23 @@ err: int romfile_loader_execute(const char *name) { struct romfile_loader_entry_s *entry; - int size, offset = 0, nfiles = 0; + int size, offset = 0, nfiles; struct romfile_loader_files *files; void *data = romfile_loadfile(name, &size); if (!data) return -1;
- /* Validate and count files */ - for (offset = 0; offset < size; offset += sizeof(*entry)) { - entry = data + offset; - /* Check that entry fits in buffer. */ - if (offset + sizeof(*entry) > size) { - warn_internalerror(); - return -1; - } - - if (le32_to_cpu(entry->command) == ROMFILE_LOADER_COMMAND_ALLOCATE) { - nfiles++; - } + if (size % sizeof(*entry)) { + warn_invalid(); + goto err; }
+ /* (over)estimate the number of files to load. */ + nfiles = size / sizeof(*entry); files = malloc_tmp(sizeof(*files) + nfiles * sizeof(files->files[0])); if (!files) { warn_noalloc(); - return -1; + goto err; } files->nfiles = 0;
@@ -177,4 +170,8 @@ int romfile_loader_execute(const char *name) free(files); free(data); return 0; + +err: + free(data); + return -1; }