[U-Boot] [PATCH] LZMA: Avoid free on null pointer.
Hi ML,
This patch has been moved out from the previous "[PATCH 0/6 v2] Support for Bootstrap Code" patchset.
This change should be safe because just add some check in order to avoid some possible free on null pointers in the LZMA decoder initialization. I noticed the error tracing all malloc/free of the code during a code profiling session.
Kindly, add this fix in the master branch.
best regards,
luigi
Luigi 'Comio' Mantellini (1): LZMA: Avoid free on null pointer.
lib/lzma/LzmaDec.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
Signed-off-by: Luigi 'Comio' Mantellini luigi.mantellini@idf-hit.com --- lib/lzma/LzmaDec.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/lzma/LzmaDec.c b/lib/lzma/LzmaDec.c index f941da2..e2dab44 100644 --- a/lib/lzma/LzmaDec.c +++ b/lib/lzma/LzmaDec.c @@ -960,7 +960,8 @@ static SRes LzmaDec_AllocateProbs2(CLzmaDec *p, const CLzmaProps *propNew, ISzAl UInt32 numProbs = LzmaProps_GetNumProbs(propNew); if (p->probs == 0 || numProbs != p->numProbs) { - LzmaDec_FreeProbs(p, alloc); + if (p->probs) + LzmaDec_FreeProbs(p, alloc); p->probs = (CLzmaProb *)alloc->Alloc(alloc, numProbs * sizeof(CLzmaProb)); p->numProbs = numProbs; if (p->probs == 0) @@ -987,7 +988,8 @@ SRes LzmaDec_Allocate(CLzmaDec *p, const Byte *props, unsigned propsSize, ISzAll dicBufSize = propNew.dicSize; if (p->dic == 0 || dicBufSize != p->dicBufSize) { - LzmaDec_FreeDict(p, alloc); + if (p->dic) + LzmaDec_FreeDict(p, alloc); p->dic = (Byte *)alloc->Alloc(alloc, dicBufSize); if (p->dic == 0) {
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
Signed-off-by: Luigi 'Comio' Mantellini luigi.mantellini@idf-hit.com
Why not move these NULL checks inside LzmaDec_FreeProbs? Then you don't have to litter the code with NULL checks and LzmaDec_FreeProbs behaves like the standard free() function.
Jocke
On Sun, Dec 5, 2010 at 11:12 AM, Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
Signed-off-by: Luigi 'Comio' Mantellini luigi.mantellini@idf-hit.com
Why not move these NULL checks inside LzmaDec_FreeProbs? Then you don't have to litter the code with NULL checks and LzmaDec_FreeProbs behaves like the standard free() function.
I don't agree with this. We have problem only on initialization and should be the caller to check if buffer are already allocated (then free them) or not. This check is on the "init behavior" and not on "robustness" of free code.
My 2cents.
luigi
Jocke
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Luigi Mantellini luigi.mantellini.ml@gmail.com wrote on 2010/12/05 11:19:58:
On Sun, Dec 5, 2010 at 11:12 AM, Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
Signed-off-by: Luigi 'Comio' Mantellini luigi.mantellini@idf-hit.com
Why not move these NULL checks inside LzmaDec_FreeProbs? Then you don't have to litter the code with NULL checks and LzmaDec_FreeProbs behaves like the standard free() function.
I don't agree with this. We have problem only on initialization and should be the caller to check if buffer are already allocated (then free them) or not. This check is on the "init behavior" and not on "robustness" of free code.
Well, that is not how malloc/free works. It would be nice if LZMA followed the same rules to minimize any confusion. There may be other places that need this check too and you end up with lots of NULL checks that just litters the code.
Jocke
On Sunday, December 05, 2010 04:18:44 Luigi 'Comio' Mantellini wrote:
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
your patch only checks p->probs, not any dictionary buffer. if you follow the code path:
... void LzmaDec_FreeProbs(CLzmaDec *p, ISzAlloc *alloc) { alloc->Free(alloc, p->probs); p->probs = 0; } ... g_Alloc.Free = SzFree; ... static void SzFree(void *p, void *address) { p = p; free(address); } ...
this only ends up doing free(p->probs) which is free(NULL) which isnt a bug.
so you're going to need to provide some more details. -mike
On Mon, Dec 6, 2010 at 8:15 AM, Mike Frysinger vapier@gentoo.org wrote:
On Sunday, December 05, 2010 04:18:44 Luigi 'Comio' Mantellini wrote:
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
your patch only checks p->probs, not any dictionary buffer. if you follow the code path:
... void LzmaDec_FreeProbs(CLzmaDec *p, ISzAlloc *alloc) { alloc->Free(alloc, p->probs); p->probs = 0; } ... g_Alloc.Free = SzFree; ... static void SzFree(void *p, void *address) { p = p; free(address); } ...
this only ends up doing free(p->probs) which is free(NULL) which isnt a bug.
In general I prefer avoid to free a null pointer, and I consider a free on a not-malloc-eted pointer a bug. The submitted patch check the pointers (p->probs and p->dict) only at init time. This is sufficient to avoid the free(NULL). The other *Free(*) calls all called on pointers that are surely not null.
The second way should be to add the null check into the FreeProbs and FreeDict functions.
best regards,
luigi
so you're going to need to provide some more details. -mike
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Monday, December 06, 2010 03:59:44 Luigi Mantellini wrote:
On Mon, Dec 6, 2010 at 8:15 AM, Mike Frysinger wrote:
On Sunday, December 05, 2010 04:18:44 Luigi 'Comio' Mantellini wrote:
On structure Initialization, LZMA code tries to free the dictionary and probs buffers, also when these are null pointers. Add some check in order to prevent the free on null pointers.
your patch only checks p->probs, not any dictionary buffer. if you follow the code path:
... void LzmaDec_FreeProbs(CLzmaDec *p, ISzAlloc *alloc) { alloc->Free(alloc, p->probs); p->probs = 0; } ... g_Alloc.Free = SzFree; ... static void SzFree(void *p, void *address) { p = p; free(address); } ...
this only ends up doing free(p->probs) which is free(NULL) which isnt a bug.
In general I prefer avoid to free a null pointer, and I consider a free on a not-malloc-eted pointer a bug.
sorry, but this is not an acceptable reason. so unless you have an actual error report here, your patch gets NAK-ed. -mike
On Mon, Dec 6, 2010 at 9:57 AM, Mike Frysinger vapier@gentoo.org wrote:
sorry, but this is not an acceptable reason. so unless you have an actual error report here, your patch gets NAK-ed. -mike
Hi mike,
my pov is different: free should (must) be called only on already allocated pointers. I know that free code checks at begin if ptr is null or not. Anyway I don't understand why a null pointer check before to call free cannot be added to the code... it's safe and follows the logical flow of the code.
I received warning from my debugger during activities on other things, and I added this fix to my code to turn-off "possible free on null pointer" warning from my debugger.
my 2Cents,
ciao ciao
luigi
On Mon, Dec 6, 2010 at 9:57 AM, Mike Frysinger vapier@gentoo.org wrote:
sorry, but this is not an acceptable reason. so unless you have an actual error report here, your patch gets NAK-ed. -mike
Hi mike,
my pov is different: free should (must) be called only on already allocated pointers. I know that free code checks at begin if ptr is null or not. Anyway I don't understand why a null pointer check before to call free cannot be added to the code... it's safe and follows the logical flow of the code.
I think you will find this is not the majority view and you will have to adapt. free(NULL) is fine and the reason for that is so that you can avoid litter the code with a ton of NULL checks.
Jocke
Hi Joakim,
We have not "ton" but just two points to fix. 1) when free dict before to allocate another one, and 2) when free probs before to allocate another one. These scenarios are not used into u-boot code, because the library is "one shot" and the other free are called (if called) just to deallocate all structures (that are surely not null).
best regards,
luigi
On Mon, Dec 6, 2010 at 10:38 AM, Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
On Mon, Dec 6, 2010 at 9:57 AM, Mike Frysinger vapier@gentoo.org wrote:
sorry, but this is not an acceptable reason. so unless you have an actual error report here, your patch gets NAK-ed. -mike
Hi mike,
my pov is different: free should (must) be called only on already allocated pointers. I know that free code checks at begin if ptr is null or not. Anyway I don't understand why a null pointer check before to call free cannot be added to the code... it's safe and follows the logical flow of the code.
I think you will find this is not the majority view and you will have to adapt. free(NULL) is fine and the reason for that is so that you can avoid litter the code with a ton of NULL checks.
Jocke
Luigi Mantellini luigi.mantellini.ml@gmail.com wrote on 2010/12/06 10:47:21:
Hi Joakim,
We have not "ton" but just two points to fix. 1) when free dict before to allocate another one, and 2) when free probs before to allocate another one. These scenarios are not used into u-boot code, because the library is "one shot" and the other free are called (if called) just to deallocate all structures (that are surely not null).
There is nothing to fix, just move on. I will and consider this matter closed.
best regards,
luigi
On Mon, Dec 6, 2010 at 10:38 AM, Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
On Mon, Dec 6, 2010 at 9:57 AM, Mike Frysinger vapier@gentoo.org wrote:
sorry, but this is not an acceptable reason. so unless you have an actual error report here, your patch gets NAK-ed. -mike
Hi mike,
my pov is different: free should (must) be called only on already allocated pointers. I know that free code checks at begin if ptr is null or not. Anyway I don't understand why a null pointer check before to call free cannot be added to the code... it's safe and follows the logical flow of the code.
I think you will find this is not the majority view and you will have to adapt. free(NULL) is fine and the reason for that is so that you can avoid litter the code with a ton of NULL checks.
Jocke
-- Luigi 'Comio' Mantellini R&D - Software Industrie Dial Face S.p.A. Via Canzo, 4 20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813 Fax: +39 02 5167 2459 web: www.idf-hit.com mail: luigi.mantellini@idf-hit.com
Dear Luigi Mantellini,
In message AANLkTi=KQ+zc7yVUe5ssc=wzZuWEcPMYQHq-c-HZ0joa@mail.gmail.com you wrote:
my pov is different: free should (must) be called only on already allocated pointers. I know that free code checks at begin if ptr is null or not. Anyway I don't understand why a null pointer check before to call free cannot be added to the code... it's safe and follows the logical flow of the code.
I received warning from my debugger during activities on other things, and I added this fix to my code to turn-off "possible free on null pointer" warning from my debugger.
free(NULL) has a well defined behaviour: "If ptr is NULL, no operation is performed."
Seems your debugger is over-cautious. This may be OK for debugging, but is no good reason to change the code.
Best regards,
Wolfgang Denk
participants (5)
-
Joakim Tjernlund -
Luigi 'Comio' Mantellini -
Luigi Mantellini -
Mike Frysinger -
Wolfgang Denk