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

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 wrote:

...

Signed-off-by: Yumei Huang --- 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