Re: [PATCH v6 2/4] util: Introduce read_file() and read_file_integer() function

[Yumei Huang <yuhuang@redhat.com>]

On Fri, Oct 24, 2025 at 11:30 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Fri, Oct 24, 2025 at 01:04:27AM +0200, Stefano Brivio wrote:
> > Sorry for the delay, mostly nits but a couple of substantial comments:
> >
> > On Fri, 17 Oct 2025 14:28:36 +0800
> > Yumei Huang <yuhuang@redhat.com> wrote:
> >
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> > > ---
> > >  util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  util.h |  8 ++++++
> > >  2 files changed, 92 insertions(+)
> > >
> > > diff --git a/util.c b/util.c
> > > index c492f90..5c8c4bc 100644
> > > --- a/util.c
> > > +++ b/util.c
> > > @@ -579,6 +579,90 @@ int write_file(const char *path, const char *buf)
> > >     return len == 0 ? 0 : -1;
> > >  }
> > >
> > > +/**
> > > + * read_file() - Read contents of file into a buffer
> > > + * @path:  File to read
> >
> > I see this is the same as write_file(), so in some sense it's
> > pre-existing, but @path isn't really a "file" in the sense that it's
> > not a file descriptor as one might expect from the description alone.
> >
> > I'd rather say "Path to file" or "Path to file to read" or something
> > like that. On the other hand, if you want to keep this consistent with
> > write_file(), never mind. Not a strong preference from me.
>
> That's a good idea, but it's not crucial to the aim of this series, so
> I'd suggest doing it as a later patch.
>
> > > + * @buf:   Buffer to store file contents
> > > + * @buf_size:      Size of buffer
> > > + *
> > > + * Return: number of bytes read on success, -1 on any error, -2 on truncation
> >
> > Similar comment here: this is partially symmetric to read_file, but
> > it's yet another convention we are introducing, because of the -2
> > special value.
> >
> > Other somewhat related functions in util.c return with a meaningful
> > errno value set, this one doesn't.
> >
> > The majority of helpers in passt, though, return with a negative
> > errno-like value, and truncation can be very well represented by
> > returning -ENOBUFS, see snprintf_check(). I think that's preferable.
> >
> > Again, if the intention is to make this consistent to write_file(), it
> > can be left as it is.
>
> Similarly.  I considered commenting earlier on the -2 or truncation -
> we don't actually use this, and it's a bit ugly.  On the other hand it
> doesn't hurt anything, so again, I think it can wait.
>
> > > +*/
> > > +ssize_t read_file(const char *path, char *buf, size_t buf_size)
> > > +{
> > > +   int fd = open(path, O_RDONLY | O_CLOEXEC);
> > > +   size_t total_read = 0;
> > > +   ssize_t rc;
> > > +
> > > +   if (fd < 0) {
> > > +           warn_perror("Could not open %s", path);
> > > +           return -1;
> > > +   }
> > > +
> > > +   while (total_read < buf_size) {
> > > +           rc = read(fd, buf + total_read, buf_size - total_read);
> > > +
> > > +           if (rc < 0) {
> > > +                   warn_perror("Couldn't read from %s", path);
> > > +                   close(fd);
> > > +                   return -1;
> > > +           }
> > > +
> > > +           if (rc == 0)
> > > +                   break;
> > > +
> > > +           total_read += rc;
> >
> > Coverity Scan (I can provide instructions separately if desired)
> > reports one issue below, but I'll mention it here for clarity: you are
> > adding 'rc', of type ssize_t, to total_read, of type size_t, and
> > buf_size is also of type size_t, so you could overflow total_read by
> > adding for example the maximum value for ssize_t twice, to it.
> >
> > We can't run into the (theoretical) issue fixed by d836d9e34586 ("util:
> > Remove possible quadratic behaviour from write_remainder()") but the
> > solution here might be similar.
> >
> > In general we should make sure that rc is less than whatever value we
> > might sum to total_read to make it overflow at any point in time.
> >
> > I didn't really check this in detail, I can do that if needed, and
> > perhaps David remembers more clearly what we did in a similar
> > situation. It might also be a false positive, by the way.
>
> I think there are two slightly overlapping issues here.
>
> 1) I'm not sure Coverity knows/trusts that read() will never return
>    more than its third argument.  That's what stops total_read from
>    ever exceeding buf_size.  I'd need to think a bit harder about how
>    to convince it that's the case.
>
> 2) buf_size is size_t, but we're returning ssize_t.  If we passed a
>    buf_size greater than ssize_t can hold, it would make a mess (UB, I
>    think).  I don't think there are any perfectly elegant solutions in
>    C, so I'd suggest:
>         ASSERT(buf_size <= SSIZE_MAX);
>
>    at the top of the function.
>
> I'd try (2) first because it's a real (if unlikely to be triggered)
> Yumei, I can walk
> you through how to install and run Coverity locally using Red Hat's
> subscription.

That would be great!

>
> [snip]
> > > +   }
> > > +
> > > +   close(fd);
> > > +
> > > +   if (total_read == buf_size) {
> > > +           warn("File %s truncated, buffer too small", path);
> >
> > The file wasn't truncated (on disk) as this comment might seem to
> > indicate. I'd rather say "File contents exceed buffer size", or
> > "Partial file read", something like that.
> >
> > While at it, you could print the size we read (it's %zu, see similar
> > examples where we print size_t types).
> >
> > > +           return -2;
> >
> > Safer to NULL-terminate also in this case, perhaps? A future caller
> > might handle -2 (or equivalent) as a "partial" failure and use the
> > buffer anyway, so not NULL-terminating it is rather subtle.
>
> That's a good idea.  Given the purpose of the function, I think a
> caller _should_ ignore the buffer if it gets an error, but it's
> worthwhile to limit the damage if a caller forgets to check.  That
> applies for other error cases too.
>
> --
> David Gibson (he or they)       | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
>                                 | around.
> http://www.ozlabs.org/~dgibson



-- 
Thanks,

Yumei Huang