Re: [Qemu-devel] [PATCH v3] qemu_file: use fwrite() correctly

[Eric Blake]

On 02/18/2014 08:56 AM, Juan Quintela wrote:
> fwrite() returns the number of items written.  But when there is one
> error, it can return a short write.
> 
> In the particular bug that I was tracking, I did a migration to a
> read-only filesystem.  And it was able to finish the migration
> correctly.  fwrite() never returned a negative error code, nor zero,
> always 4096. (migration writes chunks of about 14000 bytes).  And it
> was able to "complete" the migration with success (yes, reading the
> file was a bit more difficult).
> 
> To add insult to injury, if your amount of memory was big enough (12GB
> on my case), it overwrote some important structure, and from them,
> malloc failed.  This check makes the problem go away.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
> 
> v3: fwrite don't set errno in all cases.

Huh? What cases would those be?

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fwrite.html is
quite clear - POSIX requires that the only time fwrite() can return a
smaller number of elements written is if it encountered a write failure
as if by failed fputc(), and since fputc() is required to set errno on
write failure, likewise, fwrite() is guaranteed to set errno on short
writes.

>> The fwrite() function shall return the number of elements successfully 
>> written, which may be less than nitems if a write error is encountered. If 
>> size or nitems is 0, fwrite() shall return 0 and the state of the stream 
>> remains unchanged. Otherwise, if a write error occurs, the error indicator 
>> for the stream shall be set, [CX] [Option Start]  and errno shall be set to 
>> indicate the error. [Option End]

> +++ b/qemu-file.c
> @@ -100,7 +100,17 @@ static int stdio_put_buffer(void *opaque, const uint8_t 
> *buf, int64_t pos,
>                              int size)
>  {
>      QEMUFileStdio *s = opaque;
> -    return fwrite(buf, 1, size, s->stdio_file);
> +    int res;
> +
> +    res = fwrite(buf, 1, size, s->stdio_file);
> +
> +    if (res != size) {
> +        if (errno) {
> +            return -errno;

Even if your claim that fwrite doesn't always set errno on short writes
were true, your patch here is wrong.  Remember, POSIX states that you
CANNOT assume the value of errno is reliable unless a function returned
an error that indicates it was set, or for certain functions (like
strtol) that are required to leave errno unchanged on success.  POSIX is
also clear that no library function (including fwrite) may explicitly
set errno to 0 - they can only preserve its former value, or set it to a
non-zero value.  Furthermore, for functions which are not required to
leave errno unchanged on success, it is perfectly acceptable for those
functions (including fwrite) to clobber errno to some other value, even
when returning success, all because you cannot rely on the errno value
except when the function returned explicit failure.

So, since fwrite is not required to keep errno unchanged on success, if
you were right that there really is a case where it is not required to
set errno on failure, then this patch has a problem - you do not pre-set
errno=0, therefore, you cannot guarantee that you are not leaking some
prior errno value from some prior function call.

NAK to this version; v2 was better.
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature