From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=GvPxeosa; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 029EE5A026F for ; Fri, 24 Oct 2025 01:04:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1761260676; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mlJzO4aWOEjNAk0mF+Wf1Ua+Da9OCeE+xQ+5yGl9G30=; b=GvPxeosaNqBl88ih+RSoLiPkVm3Ua6OiVuYE/DMi9/GPyeJ3gcMxEPCEHiAHqxGpoVmfbP FGqW44sU41j8QdS581B3a8TVvgrxS61pF2CzA9ZstnNbByK40vURPQpl1M28XBR8SOqfXH /41pENlg03pKKP3qAt2B2DL5jAnMuAg= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-221-hz-7SEKMNC2q_gsLPJujDg-1; Thu, 23 Oct 2025 19:04:33 -0400 X-MC-Unique: hz-7SEKMNC2q_gsLPJujDg-1 X-Mimecast-MFC-AGG-ID: hz-7SEKMNC2q_gsLPJujDg_1761260671 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-426ce339084so1455790f8f.0 for ; Thu, 23 Oct 2025 16:04:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1761260671; x=1761865471; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=mlJzO4aWOEjNAk0mF+Wf1Ua+Da9OCeE+xQ+5yGl9G30=; b=CBCOciiXECHoctXKC3AuiDlvYan/RcEC65TiTkEUL9YKYmRH24ncdalyFukdTMwwhZ qMVs4JAKcbNGE2SBzRxqZfjw8PCL5xtHNtagsHqQA2haPGeE67wIDZWMWlg46U1gjoAO RxakZnmgqGhHRsQKUKWhtXyvi/slhlhTX8jmPoN7WXnYZZVEJc69sw/dVLVxc6MysOiK jqFaJlP2vtGeJ+AqE6bzhWYAkXOvG2nrB0Fh038Nzoe0/+G6x+mMSnbh6PnXZ2YRuxlg U/SGosaZK0zIkU+GNVogw9/HUrW199adNM3xgKkW3jcL1On+I8r1EsERkza9Nsmioz3I 37kw== X-Gm-Message-State: AOJu0YzW/KmZXgUqHrfFBcGwyOYeh3nnJSOmqiycDfPr0tbzBFnSQT9Q O030eLOLY49mqNc/v0ks+gXhVwcAG5AT3hPSsfqPEer2CAUwqTLZPpQZ9Ap2gIsPWT++ogeVcDx cOqdauj5FvSZBQjRZSCgM0A0mWTGV9BgZEUZgE+XWjLV9N/Y164dQTg== X-Gm-Gg: ASbGncujBicFwcfHVHwyP4hgwZYOmml4Gi3o/Kvz1XfShPmlly+i1vb/U1MClqAfSeB t6w3KxBwd73faiqr0/sRymAjD2DjbFwlxnMyvmeCWU86KZgm5iYlj74no/55ZOHEy/wB7I2fMdG 5tSkWJ7dTKinQwt6VTd5Fh1EqctDM694uQprhPPtWwKTMw++/tarczvU/wEC/Sb0cYIi+OVDne6 iYij1YmYet71RjSOcIVylDMyhYKCPxDQMf+XJRdORTqbKYOsUtBOF+U78xTE3jIbCgXalbCZ0+b NirwMcjq2gOmJIdYX9UG1Dw8A6wdtiW2Ga4TQYAwlTJzQL/SiRdBC5RAZ/Mr/6AUV+wkvSuWAKZ 3QZBTEReqDA== X-Received: by 2002:a05:6000:2301:b0:428:3e62:3221 with SMTP id ffacd0b85a97d-4283e6236admr14279695f8f.51.1761260670991; Thu, 23 Oct 2025 16:04:30 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHXw4QoGvetjcEQudnC8flFNcxaLw9ym7D5gpJC/4QQC0GP/MjgcRp2Ae0WAV5DZGFTmlFelQ== X-Received: by 2002:a05:6000:2301:b0:428:3e62:3221 with SMTP id ffacd0b85a97d-4283e6236admr14279672f8f.51.1761260670248; Thu, 23 Oct 2025 16:04:30 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-429897f52aasm6032414f8f.12.2025.10.23.16.04.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Oct 2025 16:04:29 -0700 (PDT) Date: Fri, 24 Oct 2025 01:04:27 +0200 From: Stefano Brivio To: Yumei Huang Subject: Re: [PATCH v6 2/4] util: Introduce read_file() and read_file_integer() function Message-ID: <20251024010427.1c8d1032@elisabeth> In-Reply-To: <20251017062838.21041-3-yuhuang@redhat.com> References: <20251017062838.21041-1-yuhuang@redhat.com> <20251017062838.21041-3-yuhuang@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 1mFP-iH5yJ805_yDvr_OG-6zjNBa6waUzV-Lf-irNoc_1761260671 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 77TCKFSYLI33ARWT2PKQ3KGKYM4WPPDU X-Message-ID-Hash: 77TCKFSYLI33ARWT2PKQ3KGKYM4WPPDU X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top, david@gibson.dropbear.id.au 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: 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. > + * @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