| Message ID | 20180713133547.21094-1-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | feaa7856f50ec5bbf843d533ee223aedd845452c |
| Headers | show |
| Series | [v2,1/3] dfu: Fix data abort in dfu_free_entities() | expand |
On Fri, Jul 13, 2018 at 4:35 PM, Sam Protsenko <semen.protsenko@linaro.org> wrote: > Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression which > described below. This patch is effectively reverting that commit, adding > corresponding comment to avoid such regressions in future. > > In case of error in dfu_config_entities(), it frees "dfu" array, which > leads to "data abort" in dfu_free_entities(), which tries to free the > same array (and even tries to access it from linked list first). The > issue occurs e.g. when partition table on device does not match > $dfu_alt_info layout: > > => dfu 0 mmc 1 > Couldn't find part #2 on mmc device #1 > DFU entities configuration failed! > data abort > > To fix this issue, do not free "dfu" array in dfu_config_entities(). It > will be freed later in dfu_free_entities(). > > Tested on BeagleBone Black (where this regression was originally found). > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > Changes in v2: > - Improve commit message by mentioning regression commit > Hi Lukasz, Can you please review and merge this whole series? Thanks! > drivers/dfu/dfu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > index e7c91193b9..a3c09334b7 100644 > --- a/drivers/dfu/dfu.c > +++ b/drivers/dfu/dfu.c > @@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char *interface, char *devstr) > ret = dfu_fill_entity(&dfu[i], s, alt_num_cnt, interface, > devstr); > if (ret) { > - free(dfu); > + /* We will free "dfu" in dfu_free_entities() */ > return -1; > } > > -- > 2.18.0 >
Hi Sam, > On Fri, Jul 13, 2018 at 4:35 PM, Sam Protsenko > <semen.protsenko@linaro.org> wrote: > > Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression > > which described below. This patch is effectively reverting that > > commit, adding corresponding comment to avoid such regressions in > > future. > > > > In case of error in dfu_config_entities(), it frees "dfu" array, > > which leads to "data abort" in dfu_free_entities(), which tries to > > free the same array (and even tries to access it from linked list > > first). The issue occurs e.g. when partition table on device does > > not match $dfu_alt_info layout: > > > > => dfu 0 mmc 1 > > Couldn't find part #2 on mmc device #1 > > DFU entities configuration failed! > > data abort > > > > To fix this issue, do not free "dfu" array in > > dfu_config_entities(). It will be freed later in > > dfu_free_entities(). > > > > Tested on BeagleBone Black (where this regression was originally > > found). > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Changes in v2: > > - Improve commit message by mentioning regression commit > > > > Hi Lukasz, > > Can you please review and merge this whole series? This has been already included: http://git.denx.de/?p=u-boot/u-boot-dfu.git;a=shortlog;h=refs/heads/master Marek has fetch this tree and will send PR soon. > > Thanks! > > > drivers/dfu/dfu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > > index e7c91193b9..a3c09334b7 100644 > > --- a/drivers/dfu/dfu.c > > +++ b/drivers/dfu/dfu.c > > @@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char > > *interface, char *devstr) ret = dfu_fill_entity(&dfu[i], s, > > alt_num_cnt, interface, devstr); > > if (ret) { > > - free(dfu); > > + /* We will free "dfu" in > > dfu_free_entities() */ return -1; > > } > > > > -- > > 2.18.0 > > 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@denx.de
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index e7c91193b9..a3c09334b7 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char *interface, char *devstr) ret = dfu_fill_entity(&dfu[i], s, alt_num_cnt, interface, devstr); if (ret) { - free(dfu); + /* We will free "dfu" in dfu_free_entities() */ return -1; }
Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression which described below. This patch is effectively reverting that commit, adding corresponding comment to avoid such regressions in future. In case of error in dfu_config_entities(), it frees "dfu" array, which leads to "data abort" in dfu_free_entities(), which tries to free the same array (and even tries to access it from linked list first). The issue occurs e.g. when partition table on device does not match $dfu_alt_info layout: => dfu 0 mmc 1 Couldn't find part #2 on mmc device #1 DFU entities configuration failed! data abort To fix this issue, do not free "dfu" array in dfu_config_entities(). It will be freed later in dfu_free_entities(). Tested on BeagleBone Black (where this regression was originally found). Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- Changes in v2: - Improve commit message by mentioning regression commit drivers/dfu/dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)