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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5719 bytes --]

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)
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).

[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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]