[PATCH v1 1/4] IOMUX: Preserve console list if realloc() fails
It's realloc() 101 to avoid `foo = realloc(foo, ...);` call due to getting a memory leak.
Actually it's not clear why realloc() has been used here. If we shrink the array, the memcpy() overwrites it anyway with the contents of a new array. If it becomes bigger, same story.
Drop useless realloc() for good and thus preserve console list in case of failed allocation.
Fixes: 16a28ef219c2 ("IOMUX: Add console multiplexing support.") Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/common/iomux.c b/common/iomux.c index 7cfd9f2e9162..cee5f266c86e 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -129,19 +129,10 @@ int iomux_doenv(const int console, const char *arg) return 1; } else { /* Works even if console_devices[console] is NULL. */ - console_devices[console] = - (struct stdio_dev **)realloc(console_devices[console], - cs_idx * sizeof(struct stdio_dev *)); - if (console_devices[console] == NULL) { - free(cons_set); - return 1; - } - memcpy(console_devices[console], cons_set, cs_idx * - sizeof(struct stdio_dev *)); - + free(console_devices[console]); + console_devices[console] = cons_set; cd_count[console] = cs_idx; } - free(cons_set); return 0; } #endif /* CONSOLE_MUX */
Obviously the following has unnecessary indentation level in 'else' branch.
if (foo) { ... return; } else { ... }
Drop indentation level by removing redundant 'else'.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/common/iomux.c b/common/iomux.c index cee5f266c86e..9fae124442c1 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -127,12 +127,12 @@ int iomux_doenv(const int console, const char *arg) if (cs_idx == 0) { free(cons_set); return 1; - } else { - /* Works even if console_devices[console] is NULL. */ - free(console_devices[console]); - console_devices[console] = cons_set; - cd_count[console] = cs_idx; } + + /* Works even if console_devices[console] is NULL. */ + free(console_devices[console]); + console_devices[console] = cons_set; + cd_count[console] = cs_idx; return 0; } #endif /* CONSOLE_MUX */
Refactor iomux_doenv() a bit in order to increase readability. There is no change in code generation on x86.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/common/iomux.c b/common/iomux.c index 9fae124442c1..8a063563aa27 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -45,15 +45,14 @@ int iomux_doenv(const int console, const char *arg) i = 0; temp = console_args; for (;;) { - temp = strchr(temp, ','); - if (temp != NULL) { - i++; - temp++; - continue; - } /* There's always one entry more than the number of commas. */ i++; - break; + + temp = strchr(temp, ','); + if (temp == NULL) + break; + + temp++; } start = (char **)malloc(i * sizeof(char *)); if (start == NULL) {
When at some point environment shrinks we need to stop dropped devices.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/common/iomux.c b/common/iomux.c index 8a063563aa27..b6936317b12e 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -22,13 +22,19 @@ void iomux_printdevs(const int console) printf("\n"); }
+static void iomux_console_stop(struct stdio_dev *dev) +{ + if (dev->stop) + dev->stop(dev); +} + /* This tries to preserve the old list if an error occurs. */ int iomux_doenv(const int console, const char *arg) { char *console_args, *temp, **start; int i, j, k, io_flag, cs_idx, repeat; + struct stdio_dev **cons_set, **old_set; struct stdio_dev *dev; - struct stdio_dev **cons_set;
console_args = strdup(arg); if (console_args == NULL) @@ -128,10 +134,23 @@ int iomux_doenv(const int console, const char *arg) return 1; }
- /* Works even if console_devices[console] is NULL. */ - free(console_devices[console]); + old_set = console_devices[console]; + repeat = cd_count[console]; + console_devices[console] = cons_set; cd_count[console] = cs_idx; + + /* Stop dropped consoles */ + for (i = 0; i < repeat; i++) { + for (j = 0; j < cs_idx; j++) { + if (old_set[i] == cons_set[j]) + break; + } + if (j == cs_idx) + iomux_console_stop(old_set[i]); + } + + free(old_set); return 0; } #endif /* CONSOLE_MUX */
On Wed, Dec 16, 2020 at 8:05 PM Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
It's realloc() 101 to avoid `foo = realloc(foo, ...);` call due to getting a memory leak.
Actually it's not clear why realloc() has been used here. If we shrink the array, the memcpy() overwrites it anyway with the contents of a new array. If it becomes bigger, same story.
Drop useless realloc() for good and thus preserve console list in case of failed allocation.
It seems more patches will come. However, this one, since it is a fix, can be applied disregarding that fact. Therefore please skip the rest of the series for now.
On Wed, Dec 16, 2020 at 09:40:30PM +0200, Andy Shevchenko wrote:
On Wed, Dec 16, 2020 at 8:05 PM Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
...
It seems more patches will come. However, this one, since it is a fix, can be applied disregarding that fact. Therefore please skip the rest of the series for now.
Okay, I have sent v2 with this included there.
participants (2)
-
Andy Shevchenko -
Andy Shevchenko