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

[Stefano Brivio <sbrivio@redhat.com>]

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.

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

> +*/
> +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.

---
/home/sbrivio/passt/util.c:625:2:
  Type: Overflowed return value (INTEGER_OVERFLOW)

/home/sbrivio/passt/util.c:596:2:
  1. path: Condition "fd < 0", taking false branch.
/home/sbrivio/passt/util.c:601:2:
  2. path: Condition "total_read < buf_size", taking true branch.
/home/sbrivio/passt/util.c:604:3:
  3. path: Condition "rc < 0", taking false branch.
/home/sbrivio/passt/util.c:610:3:
  4. path: Condition "rc == 0", taking false branch.
/home/sbrivio/passt/util.c:614:2:
  5. path: Jumping back to the beginning of the loop.
/home/sbrivio/passt/util.c:601:2:
  6. path: Condition "total_read < buf_size", taking true branch.
/home/sbrivio/passt/util.c:602:3:
  7. tainted_data_return: Called function "read(fd, buf + total_read, buf_size - total_read)", and a possible return value may be less than zero.
/home/sbrivio/passt/util.c:602:3:
  8. assign: Assigning: "rc" = "read(fd, buf + total_read, buf_size - total_read)".
/home/sbrivio/passt/util.c:604:3:
  9. path: Condition "rc < 0", taking false branch.
/home/sbrivio/passt/util.c:610:3:
  10. path: Condition "rc == 0", taking false branch.
/home/sbrivio/passt/util.c:613:3:
  11. overflow: The expression "total_read" is considered to have possibly overflowed.
/home/sbrivio/passt/util.c:614:2:
  12. path: Jumping back to the beginning of the loop.
/home/sbrivio/passt/util.c:601:2:
  13. path: Condition "total_read < buf_size", taking true branch.
/home/sbrivio/passt/util.c:604:3:
  14. path: Condition "rc < 0", taking false branch.
/home/sbrivio/passt/util.c:610:3:
  15. path: Condition "rc == 0", taking true branch.
/home/sbrivio/passt/util.c:611:4:
  16. path: Breaking from loop.
/home/sbrivio/passt/util.c:618:2:
  17. path: Condition "total_read == buf_size", taking false branch.
/home/sbrivio/passt/util.c:625:2:
  18. return_overflow: "total_read", which might have overflowed, is returned from the function.
---

> +	}
> +
> +	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.

> +	}
> +
> +	buf[total_read] = '\0';

This is not in the comment to the function ("Read contents of file into
NULL-terminated buffer"?).

> +
> +	return total_read;
> +}
> +
> +/**
> + * read_file_integer() - Read an integer value from a file
> + * @path: File to read
> + * @fallback: Default value if file can't be read

These need to be aligned in the usual way (tabs).

> + *
> + * Return: Integer value, fallback on failure

That would be @fallback if you mean 'fallback' the argument.

See also: 9e0423e13541 ("style: Fix 'Return' comment style").

> +*/
> +intmax_t read_file_integer(const char *path, intmax_t fallback)
> +{
> +	char buf[INTMAX_STRLEN];
> +	ssize_t bytes_read;
> +	intmax_t value;
> +	char *end;
> +
> +	bytes_read = read_file(path, buf, sizeof(buf));
> +
> +	if (bytes_read < 0)
> +		return fallback;
> +
> +	if (bytes_read == 0) {
> +		debug("Empty file %s", path);
> +		return fallback;
> +	}
> +
> +	errno = 0;
> +	value = strtoimax(buf, &end, 10);
> +	if (*end && *end != '\n') {
> +		debug("Invalid format in %s", path);

It took me a bit to understand that you mean that the file contains
non-numbers except for '-'. Perhaps mention that explicitly, otherwise
it looks like the file needs to have a somewhat special format? Say,
"Non-numeric content in %s".

> +		return fallback;
> +	}
> +	if (errno) {
> +		debug("Invalid value in %s: %s", path, buf);

I just learnt from strtoimax(3) that this can only be an underflow or
overflow. Maybe mention that ("Out of range value in..."), instead of
"invalid", which is kind of arbitrary?

> +		return fallback;
> +	}
> +
> +	return value;
> +}
> +
>  #ifdef __ia64__
>  /* Needed by do_clone() below: glibc doesn't export the prototype of __clone2(),
>   * use the description from clone(2).
> diff --git a/util.h b/util.h
> index 22eaac5..3f9f296 100644
> --- a/util.h
> +++ b/util.h
> @@ -222,6 +222,8 @@ void pidfile_write(int fd, pid_t pid);
>  int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);
>  int write_file(const char *path, const char *buf);
> +ssize_t read_file(const char *path, char *buf, size_t buf_size);
> +intmax_t read_file_integer(const char *path, intmax_t fallback);
>  int write_all_buf(int fd, const void *buf, size_t len);
>  int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip);
>  int read_all_buf(int fd, void *buf, size_t len);
> @@ -250,6 +252,12 @@ static inline const char *af_name(sa_family_t af)
>  
>  #define UINT16_STRLEN		(sizeof("65535"))
>  
> +/* Each byte expands to at most 3 decimal digits since 0xff == 255.
> + * Plus 2 extra bytes for the sign and null terminator.
> + * See https://stackoverflow.com/a/10536254.
> + */
> +#define INTMAX_STRLEN		(sizeof(intmax_t) * 3 + 2)
> +

As discussed, we can probably use BUFSIZ instead, and keep this simple.

>  /* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */
>  #define SOCKADDR_INET_STRLEN					\
>  	(INET_ADDRSTRLEN-1 + UINT16_STRLEN-1 + sizeof(":"))

-- 
Stefano