[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [rhel5-branch 3/5] Compute size of modules buffer in loader (#484092)



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 redhat com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list


--
David Cantrell <dcantrell redhat com>
Red Hat / Honolulu, HI


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]