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

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 28 Oct 2025 15:11:32 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> 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.  
> 
> Thank you. As I have to respin and this a minor change, I will update
> that in v7.

Just for clarity: I think David meant that it *should* be done as a
separate patch (maybe in the same series, if you have a moment, but it's
not that critical).

I don't have a particular preference, but David's point makes sense to
me.

> > > > + * @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.  
> 
> Same here.

Same as above ("I think it can wait"). Well, up to you, really.

> > > > +*/
> > > > +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)
> > bug.  Then we can see if Coverity still complains (Yumei, I can walk
> > you through how to install and run Coverity locally using Red Hat's
> > subscription).  
> 
> Coverity doesn't complain about it in my setup. Stefano may give more
> info on that. In a word, it's a false positive.

Right, my bad, it turns out I was using an outdated version and this
false positive doesn't appear anymore.

> > [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.  
> 
> The rest will update in v7 as well.

Thanks.

-- 
Stefano