Patchwork [U-Boot] LZMA: Avoid free on null pointer.

login
register
mail settings
Submitter luigi.mantellini@idf-hit.com
Date Dec. 5, 2010, 9:18 a.m.
Message ID <1291540724-26513-2-git-send-email-luigi.mantellini@idf-hit.com>
Download mbox | patch
Permalink /patch/74290/
State Rejected
Headers show

Comments

luigi.mantellini@idf-hit.com - Dec. 5, 2010, 9:18 a.m.
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(-)
Joakim Tjernlund - Dec. 5, 2010, 10:12 a.m.
>
> 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
Luigi Mantellini - Dec. 5, 2010, 10:19 a.m.
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
>
Joakim Tjernlund - Dec. 5, 2010, 10:27 a.m.
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
Mike Frysinger - Dec. 6, 2010, 7:15 a.m.
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
Mike Frysinger - Dec. 6, 2010, 8:57 a.m.
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
Luigi Mantellini - Dec. 6, 2010, 8:59 a.m.
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
>
>
Luigi Mantellini - Dec. 6, 2010, 9:13 a.m.
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
Joakim Tjernlund - Dec. 6, 2010, 9:38 a.m.
>
> 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 - Dec. 6, 2010, 9:47 a.m.
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
>
>
Joakim Tjernlund - Dec. 6, 2010, 9:54 a.m.
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
Wolfgang Denk - Dec. 6, 2010, 12:31 p.m.
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

Patch

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)
     {