Re: [PATCH 4/5] Src/module: fix use-after-free in setmathfuncs()

[Kamil Dudka <kdudka@xxxxxxxxxx>]

On Wednesday, November 7, 2018 3:21:31 PM CET Mikael Magnusson wrote:
> On 11/7/18, Kamil Dudka <kdudka@xxxxxxxxxx> wrote:
> > Detected by Coverity Analysis:
> > 
> > Error: USE_AFTER_FREE (CWE-825):
> > zsh-5.5.1/Src/module.c:1390: freed_arg: "deletemathfunc" frees "f".
> > zsh-5.5.1/Src/module.c:1352:6: freed_arg: "zfree" frees parameter "f".
> > zsh-5.5.1/Src/mem.c:1888:5: freed_arg: "free" frees parameter "p".
> > zsh-5.5.1/Src/module.c:1394: deref_after_free: Dereferencing freed pointer
> > "f".
> > 1392|   		ret = 1;
> > 1393|   	    } else {
> > 1394|-> 		f->flags &= ~MFF_ADDED;
> > 1395|   	    }
> > 1396|   	}
> > ---
> > 
> >  Src/module.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/Src/module.c b/Src/module.c
> > index 4ae78310f..33d75ebbd 100644
> > --- a/Src/module.c
> > +++ b/Src/module.c
> > @@ -1390,8 +1390,6 @@ setmathfuncs(char const *nam, MathFunc f, int size,
> > int *e)
> > 
> >  	    if (deletemathfunc(f)) {
> >  		
> >  		zwarnnam(nam, "math function `%s' already deleted", f->name);
> >  		ret = 1;
> > 
> > -	    } else {
> > -		f->flags &= ~MFF_ADDED;
> > 
> >  	    }
> >  	
> >  	}
> >  	f++;
> > 
> > --
> > 2.17.2
> 
> In the other branch, if f was already deleted, how can we use f->name there?

Do you mean deleted by the call of deletemathfunc(f) from setmathfuncs()?

If deletemathfunc(f) returns -1, it did not delete anything.

If deletemathfunc(f) returns 0, it either called free(f) or already executed:

    f->flags &= ~MFF_ADDED

Executing it in setmathfuncs() again could hardly do anything useful.
It would only cause use-after-free in case f->module was true.

Kamil