On 05/03/2009 02:42 AM, Hans de Goede wrote:
Hi, Comments inline. On 05/01/2009 11:22 PM, David Cantrell wrote:In hardware.c, we build a colon-separated list of module names. The buffer storing that string was set to a hard limit of 1024, which isn't big enough for certain systems. This patch changes the function to calculate the length required of the buffer and resize it with realloc(). --- loader2/hardware.c | 34 ++++++++++++++++++++++++++++------ 1 files changed, 28 insertions(+), 6 deletions(-) diff --git a/loader2/hardware.c b/loader2/hardware.c index 2cea886..7c574eb 100644 --- a/loader2/hardware.c +++ b/loader2/hardware.c @@ -247,9 +247,9 @@ int earlyModuleLoad(moduleInfoSet modInfo, moduleList modLoaded, int busProbe(moduleInfoSet modInfo, moduleList modLoaded, moduleDeps modDeps, int justProbe) { - int i; + int i, len = 0; char ** modList; - char modules[1024]; + char *modules = NULL; /* we always want to try to find out about pcmcia controllers even * if using noprobe */Given that modules and len are only used in the "else if (modList) {" block, I think it would be best to move the declaritions to that block to make this clear.@@ -269,17 +269,39 @@ int busProbe(moduleInfoSet modInfo, moduleList modLoaded, moduleDeps modDeps, } else if (modList) { int probeVirtioAgain = 0; - *modules = '\0'; - + /* compute length of colon-separated string */ + for (i = 0; modList[i]; i++) { + if (i) { + len += 1; /* ':' between each module name */ + } + + len += strlen(modList[i]); + } + + len += 1; /* '\0' at the end */ +Ok.for (i = 0; modList[i]; i++) { - if (i) strcat(modules, ":"); - strcat(modules, modList[i]); + if (i> 0) { + modules = strncat(modules, ":", 1); + } + + if (modules == NULL) { + modules = strdup(modList[i]); + + if ((modules = realloc(modules, len)) == NULL) { + logMessage(ERROR, "error building modules string"); + return 0; + } + } else { + modules = strncat(modules, modList[i], strlen(modList[i])); + }This seems rather complicated, why first do a strdup and then a realloc ? why not just do a malloc, followed by a "modules[0] = 0;" and make the strncat unconditionally? Also why do the alloc conditionally in the loop? Why not simply do it unconditionally before the loop?
Bleh, it is unnecessarily complicated. I've changed it around to a single malloc() rather than realloc() throughout the loop. Not sure how I got there in the first place. Updated patch coming in another thread.
Last there is no need to use strncat here, a simple strcat will suffice, actually the strncat is harmfully because we now do not 0 terminate the string, so if the memory block we got is not zero'd out we may have an unterminated string. Regards, Hans _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
-- David Cantrell <dcantrell@xxxxxxxxxx> Red Hat / Honolulu, HI _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
