[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat
From: |
Kamil Dudka |
Subject: |
Re: [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat on a NULL pointer |
Date: |
Mon, 08 Feb 2016 09:35:04 +0100 |
User-agent: |
KMail/4.14.10 (Linux/4.3.4-300.fc23.x86_64; KDE/4.14.16; x86_64; ; ) |
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
- [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat on a NULL pointer, Kamil Dudka, 2016/02/03
- [Nano-devel] [PATCH 2/2] use futimens() if available, instead of utime(), Kamil Dudka, 2016/02/03
- Re: [Nano-devel] [PATCH 2/2] use futimens() if available, instead of utime(), Mike Frysinger, 2016/02/03
- Re: [Nano-devel] [PATCH 2/2] use futimens() if available, instead of utime(), Kamil Dudka, 2016/02/03
- [Nano-devel] begin using gnulib?, Benno Schulenberg, 2016/02/04
- Re: [Nano-devel] begin using gnulib?, Mike Frysinger, 2016/02/04
- Re: [Nano-devel] begin using gnulib?, Chris Allegretta, 2016/02/05
- Re: [Nano-devel] begin using gnulib?, Kamil Dudka, 2016/02/05
Re: [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat on a NULL pointer, Benno Schulenberg, 2016/02/06
- Re: [Nano-devel] [PATCH 1/2] check stat's result and avoid calling stat on a NULL pointer,
Kamil Dudka <=