[PATCH] cpio_utils: Return error on short reads

[Christian Storm]

Return -ENODATA when the actual bytes read are less than
expected, i.e., a premature EOF or short read happened.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
core/cpio_utils.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index 4a8b2964..adb2b277 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -83,6 +83,12 @@ static int _fill_buffer(int fd, unsigned char *buf, unsigned int nbytes, unsigne
return -EFAULT;
}
if (len == 0) {
+ if (count > 0) {
+ ERROR("Short read in stream %d: "
+ "got %lu of %lu bytes",
+ fd, count, count + nbytes);
+ return -ENODATA;
+ }
return count;
}
if (checksum)
--
2.55.0

[Stefano Babic]

Hi Christian,

This is not clear to - let's say we have an EOF, the function is still
reading some bytes and it returns with count exactly the number of bytes
available. It is then duty of the core to process the bytes and says
that they are not enough.

Which is the reason for this ? Is there a real issue or was it just by
reading the code ?

Best regards,
Stefano


--
_______________________________________________________________________
Nabla Software Engineering GmbH
Hirschstr. 111A | 86156 Augsburg | Tel: +49 821 45592596
Geschäftsführer : Stefano Babic | HRB 40522 Augsburg
E-Mail: sba...@nabladev.com

[Storm, Christian]

Hi Stefano,

thanks for looking into it!

>> Return -ENODATA when the actual bytes read are less than
>> expected, i.e., a premature EOF or short read happened.
>> Signed-off-by: Christian Storm <christi...@siemens.com>
>> ---
>> core/cpio_utils.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c
>> index 4a8b2964..adb2b277 100644
>> --- a/core/cpio_utils.c
>> +++ b/core/cpio_utils.c
>> @@ -83,6 +83,12 @@ static int _fill_buffer(int fd, unsigned char *buf, unsigned int nbytes, unsigne
>> return -EFAULT;
>> }
>> if (len == 0) {
>> + if (count > 0) {
>> + ERROR("Short read in stream %d: "
>> + "got %lu of %lu bytes",
>> + fd, count, count + nbytes);
>> + return -ENODATA;
>> + }
>> return count;
>> }
>> if (checksum)
>
> This is not clear to - let's say we have an EOF, the function is still reading some bytes and it returns with count exactly the number of bytes available.

Let me explain my thought process here, and please correct me if I'm off-track:

On an initial EOF in the first iteration, len == 0 and count == 0, resulting in return count == 0 which is fine. There simply was no data – which is a condition callers should be prepared for.

On a second+ iteration, when encountering EOF, len == 0 and count > 0 as some bytes have already been read – but less than the required nbytes as that would've regularly exited the while nbytes > 0 loop.
So, here, the caller requested nbytes bytes but only got count < nbytes actually read, i.e., a short read.

> It is then duty of the core to process the bytes and says that they are not enough.

Fair question. If you look at, e.g., extract_cpio_header(), that one expects to (exactly) read
sizeof(struct new_ascii_header)
= 110 bytes. The return value is checked for errors (i.e., _fill_buffer() returned < 0) but not if less than the expected number of bytes (i.e. < 110 or 0 = initial EOF) have been read, i.e., there were some bytes read but not enough. Now, say, 3 bytes were read. Then, e.g., get_cpiohdr() just doing a cast
struct new_ascii_header *cpiohdr = (struct new_ascii_header *)buf;
potentially works on uninitialized or wrong/outdated memory.

Now, one can argue whether that should be handled at the caller sites or in _fill_buffer(). I chose _fill_buffer() as no caller site currently checks for this condition – and neither for the initial EOF case. That said, I'm also fine with putting this check on all the caller sites though...

> Which is the reason for this ? Is there a real issue or was it just by reading the code ?

I stumbled over this one with an, let's say, interesting cpio file. So no real issue but more or less just by reading the code.


Best regards,
Christian

--
Dr. Christian Storm
Siemens AG, FT RPD CED
Friedrich-Ludwig-Bauer-Str. 3, 85748 Garching, Germany

[Dominique MARTINET]

'Storm, Christian' via swupdate wrote on Wed, Jul 22, 2026 at 10:58:15AM +0000:
> Let me explain my thought process here, and please correct me if I'm off-track:
>
> On an initial EOF in the first iteration, len == 0 and count == 0, resulting in return count == 0 which is fine. There simply was no data – which is a condition callers should be prepared for.
>
> On a second+ iteration, when encountering EOF, len == 0 and count > 0 as some bytes have already been read – but less than the required nbytes as that would've regularly exited the while nbytes > 0 loop.
> So, here, the caller requested nbytes bytes but only got count <
> nbytes actually read, i.e., a short read.

I think this (erroring out in _fill_buffer) is fine, input_step() also
expects full reads before doing digest and things like that, and there
are external callers like fill_buffer() in stream_interface.c that do as
well.
From what I can see most callers aren't checking for == 0 but only < 0,
so I'd go as far as erroring out if count == 0 as well, or we ought to
check all call sites (in which case we could make them handle short
reads as well)


I'd reword the error message though: from the fill_buffer caller point
of view we'd return a short read, but the reason behind the short read
is "early EOF" so we should say so.
I think it's a vocabulary thing, because you seem to understand
returning 0 means EOF, but short read is connotated towards "I asked for
n bytes but got m < n bytes" - which is perfectly fine and something we
handle here by retrying, but historically the VFS has been looping
internally so short reads are rather rare and a source of bugs for
programs less careful.
(but that's just my filesystem dev background speaking here...)


Thanks for finding this!
--
Dominique

[Storm, Christian]

>> Let me explain my thought process here, and please correct me if I'm off-track:
>>
>> On an initial EOF in the first iteration, len == 0 and count == 0, resulting in return count == 0 which is fine. There simply was no data – which is a condition callers should be prepared for.
>>
>> On a second+ iteration, when encountering EOF, len == 0 and count > 0 as some bytes have already been read – but less than the required nbytes as that would've regularly exited the while nbytes > 0 loop.
>> So, here, the caller requested nbytes bytes but only got count <
>> nbytes actually read, i.e., a short read.
>
> I think this (erroring out in _fill_buffer) is fine, input_step() also
> expects full reads before doing digest and things like that, and there
> are external callers like fill_buffer() in stream_interface.c that do as
> well.
> From what I can see most callers aren't checking for == 0 but only < 0,
> so I'd go as far as erroring out if count == 0 as well, or we ought to
> check all call sites (in which case we could make them handle short
> reads as well)

Exactly, this would've been my follow-up patch(es) :)
But I won't go as fas as saying count == 0 is an error as that's the expected outcome when reading an empty file. The callers should check for this condition though. And it depends on the intended semantics and usage of the _fill_buffer() function whether this is an error or not...

> I'd reword the error message though: from the fill_buffer caller point
> of view we'd return a short read, but the reason behind the short read
> is "early EOF" so we should say so.

Sure, no strong feelings here, "Early EOF encountered" would be fine with me as well.

> I think it's a vocabulary thing, because you seem to understand
> returning 0 means EOF, but short read is connotated towards "I asked for
> n bytes but got m < n bytes"

Exactly.

> - which is perfectly fine and something we handle here by retrying,

That depends on what the semantics is/should be and who has the obligation to handle this. Erroring out would also be fine as is retrying.

> but historically the VFS has been looping
> internally so short reads are rather rare and a source of bugs for
> programs less careful.
> (but that's just my filesystem dev background speaking here...)

Hehe, been there, done that as well :) The OS's job being an abstraction over hardware is to make it as convenient and bullet-proof as possible to the programmer...


Kind regards,

[Stefano Babic]

On 7/23/26 04:19, Dominique MARTINET wrote:
> 'Storm, Christian' via swupdate wrote on Wed, Jul 22, 2026 at 10:58:15AM +0000:
>> Let me explain my thought process here, and please correct me if I'm off-track:
>>
>> On an initial EOF in the first iteration, len == 0 and count == 0, resulting in return count == 0 which is fine. There simply was no data – which is a condition callers should be prepared for.
>>
>> On a second+ iteration, when encountering EOF, len == 0 and count > 0 as some bytes have already been read – but less than the required nbytes as that would've regularly exited the while nbytes > 0 loop.
>> So, here, the caller requested nbytes bytes but only got count <
>> nbytes actually read, i.e., a short read.
>
> I think this (erroring out in _fill_buffer) is fine, input_step() also
> expects full reads before doing digest and things like that, and there
> are external callers like fill_buffer() in stream_interface.c that do as
> well.
> From what I can see most callers aren't checking for == 0 but only < 0,
> so I'd go as far as erroring out if count == 0 as well, or we ought to
> check all call sites (in which case we could make them handle short
> reads as well)

You both are right - the caller is expecting to get the requested N
bytes, and if this does not happens, something went wro9ng, that is an
error should be raised. Got it.

>
>
> I'd reword the error message though: from the fill_buffer caller point
> of view we'd return a short read, but the reason behind the short read
> is "early EOF" so we should say so.
> I think it's a vocabulary thing, because you seem to understand
> returning 0 means EOF, but short read is connotated towards "I asked for
> n bytes but got m < n bytes" - which is perfectly fine and something we
> handle here by retrying, but historically the VFS has been looping
> internally so short reads are rather rare and a source of bugs for
> programs less careful.
> (but that's just my filesystem dev background speaking here...)
>
>
> Thanks for finding this!

Thanks both !

So I just expecting a V2 with the reworked commit, and then I will apply it.

[Christian Storm]

Return -ENODATA from _fill_buffer() when the actual bytes
read are less than expected, i.e., an early EOF / short
read happened.

Error logging and handling is moved to the caller sites
to enable individual actions. With this, also handle
initial EOF reads at the caller sites individually.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

core/cpio_utils.c | 35 ++++++++++++++++++++++++++++-------
core/stream_interface.c | 5 +++--
2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index 4a8b2964..e8e61ddb 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -9,6 +9,7 @@
#include <stdlib.h>
#include <inttypes.h>
#include <stdio.h>
+#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
@@ -78,12 +79,10 @@ static int _fill_buffer(int fd, unsigned char *buf, unsigned int nbytes, unsigne
if (errno == EINTR) {
continue;
}
-
- ERROR("Failure in stream %d: %s", fd, strerror(errno));
- return -EFAULT;
+ return -errno;
}
if (len == 0) {
- return count;
+ return count > 0 ? -ENODATA : 0;
}
if (checksum)
for (i = 0; i < len; i++)
@@ -258,8 +257,18 @@ static int input_step(void *state, void *buffer, size_t size)
case INPUT_FROM_FD:
ret = _fill_buffer(s->fdin, buffer, size, s->offs, &s->checksum, s->dgst);
if (ret < 0) {
+ ERROR("Error reading %zu bytes from stream %d: %s",
+ size, s->fdin, strerror(-ret));
return ret;
}
+ if (ret == 0) {
+ if (s->nbytes > 0) {
+ ERROR("Early EOF in stream %d: %zu bytes missing",
+ s->fdin, s->nbytes);
+ return -ENODATA;
+ }
+ return 0;
+ }
break;
case INPUT_FROM_MEMORY:
memcpy(buffer, &s->inbuf[s->pos], size);
@@ -824,6 +833,9 @@ int copyfile(struct swupdate_copy *args)
#endif

for (;;) {
+ /*
+ * Note: buffer size is clamped in input_step().
+ */
ret = step(state, buffer, sizeof buffer);
if (ret == -EAGAIN) {
continue;
@@ -929,8 +941,12 @@ int copyimage(void *out, struct img_type *img, writeimage callback)
int extract_cpio_header(int fd, struct filehdr *fhdr, unsigned long *offset)
{
unsigned char buf[sizeof(fhdr->filename)];
- if (_fill_buffer(fd, buf, sizeof(struct new_ascii_header), offset, NULL, NULL) < 0)
+ int ret;
+ if ((ret = _fill_buffer(fd, buf, sizeof(struct new_ascii_header), offset, NULL, NULL)) <= 0) {
+ ERROR("Cannot read CPIO header from stream %d: %s",
+ fd, ret ? strerror(-ret) : "Early EOF");
return -EINVAL;
+ }
if (get_cpiohdr(buf, fhdr) < 0) {
ERROR("CPIO Header corrupted, cannot be parsed");
return -EINVAL;
@@ -943,14 +959,19 @@ int extract_cpio_header(int fd, struct filehdr *fhdr, unsigned long *offset)
return -EINVAL;
}

- if (_fill_buffer(fd, buf, fhdr->namesize , offset, NULL, NULL) < 0)
+ if ((ret = _fill_buffer(fd, buf, fhdr->namesize , offset, NULL, NULL)) <= 0) {
+ ERROR("Cannot read CPIO filename from stream %d: %s",
+ fd, ret ? strerror(-ret) : "Early EOF");
return -EINVAL;
+ }
buf[fhdr->namesize] = '\0';
strlcpy(fhdr->filename, (char *)buf, sizeof(fhdr->filename));

/* Skip filename padding, if any */
- if (_fill_buffer(fd, buf, (4 - (*offset % 4)) % 4, offset, NULL, NULL) < 0)
+ if ((ret = _fill_buffer(fd, buf, NPAD_BYTES(*offset), offset, NULL, NULL)) < 0) {
+ ERROR("Cannot read padding bytes from stream %d: %s", fd, strerror(-ret));
return -EINVAL;
+ }

return 0;
}
diff --git a/core/stream_interface.c b/core/stream_interface.c
index 6764d84e..935384fb 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -479,8 +479,9 @@ static int save_stream(int fdin, struct swupdate_cfg *software)
*/
while (files-- > 0) {
len = fill_buffer(fdin, buf, sizeof(struct new_ascii_header));
- if (len < 0) {
- ERROR("Reading from file failed, error %d", errno);
+ if (len <= 0) {
+ ERROR("Cannot read CPIO header from stream %d: %s",
+ fdin, len ? strerror(-len) : "Early EOF");
ret = -EFAULT;
goto no_copy_output;
}
--
2.55.0

[Christian Storm]

Since cpio checksums are defined over file data only, do not
accumulate padding bytes into the checksum, in particular as
args->checksum is overwritten and assigned the input step's
checksum right below anyway, immediately discarding whatever
the padding read has added to the checksum.

Signed-off-by: Christian Storm <christi...@siemens.com>
---

core/cpio_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index e8e61ddb..9faa8019 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -875,7 +875,7 @@ int copyfile(struct swupdate_copy *args)

if (!args->inbuf) {
ret = _fill_buffer(args->fdin, buffer, NPAD_BYTES(*args->offs),
- args->offs, args->checksum, NULL);
+ args->offs, NULL, NULL);
if (ret < 0)
DEBUG("Padding bytes are not read, ignoring");
}
--
2.55.0

[Stefano Babic]

Applied to -master, thanks !

Best regards,
Stefano Babic

[Stefano Babic]

On 8/3/26 22:49, 'Christian Storm' via swupdate wrote:
> Return -ENODATA from _fill_buffer() when the actual bytes
> read are less than expected, i.e., an early EOF / short
> read happened.
>
> Error logging and handling is moved to the caller sites
> to enable individual actions. With this, also handle
> initial EOF reads at the caller sites individually.
>
> Signed-off-by: Christian Storm <christi...@siemens.com>
> ---

Applied to -master, thanks !

Best regards,
Stefano Babic