| Message ID | 1291540724-26513-2-git-send-email-luigi.mantellini@idf-hit.com |
|---|---|
| State | Rejected |
| Headers | show |
> > 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 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 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 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
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> --- lib/lzma/LzmaDec.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)