From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202510 header.b=WomG408u; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id B59515A061A for ; Fri, 24 Oct 2025 05:30:33 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1761276629; bh=+YtGl4u78yagXJUIwu+WSQkn3KoQVoWTjBf4Bke9Mcs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WomG408uEEvumR8M6aWEVdz3Pcqjt8Ow1anGGUeX/Y93yr+vMKmWg5K4vww6q9dyE CJHE7BSSaxGXGEb7XIhRHN+kf8cYMOBTYpZV51hiAbYD+81aBdX9y80hpDAq92uM/Q zfsaAsztQ/CGvnuJGPQTDyKZNPSTBaKbiNuiX3iafKJv6e0oQN9oIGZeHTljg90nSj gQaGag2X3++hA/C2TiyWI29+HL+thn02BYhNVMIPpT3wzoNPBoC7Jz56x7SlhHVRvJ snOonsQdOu+xToetaRy4R0kiCQCSC91Kf6tZKsQTmi33m/clOWcsKuCJUFHc5VKjfZ /hjsfwU/Jl/HA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ct7hP714rz4wM5; Fri, 24 Oct 2025 14:30:29 +1100 (AEDT) Date: Fri, 24 Oct 2025 14:16:02 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v6 2/4] util: Introduce read_file() and read_file_integer() function Message-ID: References: <20251017062838.21041-1-yuhuang@redhat.com> <20251017062838.21041-3-yuhuang@redhat.com> <20251024010427.1c8d1032@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="UwBTQqjEdl6WW0uk" Content-Disposition: inline In-Reply-To: <20251024010427.1c8d1032@elisabeth> Message-ID-Hash: BEMRDTVKCKK3IDTPZI4QFNKH6GZG3O5D X-Message-ID-Hash: BEMRDTVKCKK3IDTPZI4QFNKH6GZG3O5D X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Yumei Huang , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --UwBTQqjEdl6WW0uk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > On Fri, 17 Oct 2025 14:28:36 +0800 > Yumei Huang wrote: >=20 > > Signed-off-by: Yumei Huang > > --- > > util.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > util.h | 8 ++++++ > > 2 files changed, 92 insertions(+) > >=20 > > 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 =3D=3D 0 ? 0 : -1; > > } > > =20 > > +/** > > + * read_file() - Read contents of file into a buffer > > + * @path: File to read >=20 > 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. >=20 > 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 tru= ncation >=20 > 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. >=20 > Other somewhat related functions in util.c return with a meaningful > errno value set, this one doesn't. >=20 > 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. >=20 > 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 =3D open(path, O_RDONLY | O_CLOEXEC); > > + size_t total_read =3D 0; > > + ssize_t rc; > > + > > + if (fd < 0) { > > + warn_perror("Could not open %s", path); > > + return -1; > > + } > > + > > + while (total_read < buf_size) { > > + rc =3D 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 =3D=3D 0) > > + break; > > + > > + total_read +=3D rc; >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 <=3D 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 =3D=3D buf_size) { > > + warn("File %s truncated, buffer too small", path); >=20 > 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. >=20 > While at it, you could print the size we read (it's %zu, see similar > examples where we print size_t types). >=20 > > + return -2; >=20 > 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. --=20 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 --UwBTQqjEdl6WW0uk Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmj672MACgkQzQJF27ox 2GdLEw/9EcN/hrhUpqY80UchuveG8TgAyT8T24QOLqeUcKTxFRlCa1aOOCjvvIfV dpFSG82Xu3jmMhxghCMYcIToyusbdMiri8E6iffSTwk36RtrlZnx+PmaW/mf7g9n eUO6F4eC6hd56DYykCNj6MSNCasONp+Ma45TbCdGKKbw9g5S57WLStRssMCW9YuU wbvRDMYtyOjoF4ol2+zf/tUvV+TZakGZwJhetLrThvJ/jeAibt4OA1M1LkJdr87X 8rzp/r+P3iOCjGK2MoAdXx9Db/XghW7gYjCBhdWX6o8xq2b5G/P5UKn0dMB3nZ6d Otau7U8aecjlsoPXbQ6af9OAGgxDFcejRIDhnexuzNobPaT9CSay3CO232zf0HAe ZMaHdP5oEmV5IhcNMHopemcWLSs0pLEtZOX1E7I8Kdx2/ijcTK6jrkD5e6NPlDOL HFzUaqygOYBvE2TvrNhlr1Lg8WRqsYcyeYxsgUzwpdc8XuwHXy3zYQO203Q9+uW8 dD/CrSGgCLVxzZQeLJu6hOhSfQWTVbBsJkvpn7DSQj0S9tAVuf+Gzl+9pvKsA+Es jX5vaCjbqtBjPS6SVSlLp9mNueRgh9369GL3H1A4JoZGBPn9f+g454nHMCm70JTY IiqixkYzL6M20oXLKULG0qfGP11KU1u7pqRTDW+59R6v8qCMCBQ= =Ppri -----END PGP SIGNATURE----- --UwBTQqjEdl6WW0uk--