Re: [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat on a NULL pointer

[Kamil Dudka]

On Saturday 06 February 2016 11:35:29 Benno Schulenberg wrote:
> On Wed, Feb 3, 2016, at 12:32, Kamil Dudka wrote:
> > diff --git a/trunk/nano/src/files.c b/trunk/nano/src/files.c
> > index a7aa1c5..cbd81bf 100644
> > --- a/trunk/nano/src/files.c
> > +++ b/trunk/nano/src/files.c
> > @@ -2214,8 +2226,10 @@ bool write_file(const char *name, FILE *f_open,
> > bool tmp, append_type> 
> >     if (openfile->current_stat == NULL)
> >     
> >         openfile->current_stat =
> >             
> >             (struct stat *)nmalloc(sizeof(struct stat));
> > 
> > -   if (!openfile->mark_set)
> > -       stat(realname, openfile->current_stat);
> > +   if (!openfile->mark_set && 0 != stat(realname, openfile-
>current_stat))
> > {
> > +       free(openfile->current_stat);
> > +       openfile->current_stat = NULL;
> > +   }
> 
> I don't get this part.  Why (also in the original code) malloc the
> stat struct when not actually going to stat()? Or asked another 
> way: why free the stat struct only when the mark is not set?

Yes, that looks suspicious because, if nmalloc() was called and stat() was 
skipped (or failed),  openfile->current_stat would point to uninitialized 
memory.

> So I think this part should be:
> 
> -       /* Update current_stat to reference the file as it is now. */
> -       if (openfile->current_stat == NULL)
> -           openfile->current_stat =
> -               (struct stat *)nmalloc(sizeof(struct stat));
> -       if (!openfile->mark_set)
> -           stat(realname, openfile->current_stat);
> +       if (!openfile->mark_set) {
> +           if (openfile->current_stat == NULL)
> +               openfile->current_stat =
> +                       (struct stat *)nmalloc(sizeof(struct stat));
> +
> +           /* Update the stat info to reflect the file as it is now. */
> +           if (stat(realname, openfile->current_stat) != 0) {
> +               free(openfile->current_stat);
> +               openfile->current_stat = NULL;
> +           }
> +       }

Looks good to me.

> But still... as the comment says, it assumes that there is
> already stat info there, so the malloc should be superfluous
> in this case.  But the whole routine is so convoluted that I
> don't dare remove it.

I do not fully understand the code either.

Kamil

> Benno