1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate
  4. View patch

[Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

Max Reitz posted 2 patches 7 years, 7 months ago
[Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Max Reitz 7 years, 7 months ago 
Storing the lseek() result in an int results in it overflowing when the
file is at least 2 GB big.  Then, we have a 50 % chance of the result
being "negative" and thus thinking an error occurred when actually
everything went just fine.

So we should use the correct type for storing the result: off_t.

Reported-by: Daniel P. Berrange <berrange@redhat.com>
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index f1591c3849..90c25864a0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
     case PREALLOC_MODE_FULL:
     {
         int64_t num = 0, left = offset - current_length;
+        off_t seek_result;
 
         /*
          * Knowing the final size from the beginning could allow the file
@@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
 
         buf = g_malloc0(65536);
 
-        result = lseek(fd, current_length, SEEK_SET);
-        if (result < 0) {
+        seek_result = lseek(fd, current_length, SEEK_SET);
+        if (seek_result < 0) {
             result = -errno;
             error_setg_errno(errp, -result,
                              "Failed to seek to the old end of file");
-- 
2.14.3
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Daniel P. Berrangé 7 years, 7 months ago 
On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
> 
> So we should use the correct type for storing the result: off_t.
> 
> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>      case PREALLOC_MODE_FULL:
>      {
>          int64_t num = 0, left = offset - current_length;
> +        off_t seek_result;
>  
>          /*
>           * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>  
>          buf = g_malloc0(65536);
>  
> -        result = lseek(fd, current_length, SEEK_SET);
> -        if (result < 0) {
> +        seek_result = lseek(fd, current_length, SEEK_SET);
> +        if (seek_result < 0) {

off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
exact value (off_t)-1 indicates an error. So this needs to be

   if (seek_result == (off_t)-1) {
      ...
   }


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Max Reitz 7 years, 7 months ago 
On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
>> Storing the lseek() result in an int results in it overflowing when the
>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
>> being "negative" and thus thinking an error occurred when actually
>> everything went just fine.
>>
>> So we should use the correct type for storing the result: off_t.
>>
>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/file-posix.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index f1591c3849..90c25864a0 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>      case PREALLOC_MODE_FULL:
>>      {
>>          int64_t num = 0, left = offset - current_length;
>> +        off_t seek_result;
>>  
>>          /*
>>           * Knowing the final size from the beginning could allow the file
>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>  
>>          buf = g_malloc0(65536);
>>  
>> -        result = lseek(fd, current_length, SEEK_SET);
>> -        if (result < 0) {
>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>> +        if (seek_result < 0) {
> 
> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> exact value (off_t)-1 indicates an error. So this needs to be
> 
>    if (seek_result == (off_t)-1) {
>       ...
>    }

Hmmm... On my system, it appears to be a long int[1].  And
find_allocation() does an off_t < 0 comparison already.  And
"man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
types."

Max


[1]

#define do_stringify(x) #x



#define stringify(x) do_stringify(x)

int main(void)
{
    printf("%s\n", stringify(__OFF_T_TYPE));
}

Output: long int
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Daniel P. Berrangé 7 years, 7 months ago 
On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> >> Storing the lseek() result in an int results in it overflowing when the
> >> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> >> being "negative" and thus thinking an error occurred when actually
> >> everything went just fine.
> >>
> >> So we should use the correct type for storing the result: off_t.
> >>
> >> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  block/file-posix.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index f1591c3849..90c25864a0 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>      case PREALLOC_MODE_FULL:
> >>      {
> >>          int64_t num = 0, left = offset - current_length;
> >> +        off_t seek_result;
> >>  
> >>          /*
> >>           * Knowing the final size from the beginning could allow the file
> >> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>  
> >>          buf = g_malloc0(65536);
> >>  
> >> -        result = lseek(fd, current_length, SEEK_SET);
> >> -        if (result < 0) {
> >> +        seek_result = lseek(fd, current_length, SEEK_SET);
> >> +        if (seek_result < 0) {
> > 
> > off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> > exact value (off_t)-1 indicates an error. So this needs to be
> > 
> >    if (seek_result == (off_t)-1) {
> >       ...
> >    }
> 
> Hmmm... On my system, it appears to be a long int[1].  And
> find_allocation() does an off_t < 0 comparison already.  And
> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> types."

Hmm, that's odd then - lseek man page explicitly said it must be cast,
which suggested to me it could be unsigned:

   RETURN VALUE
       Upon successful completion, lseek() returns the resulting offset  loca‐
       tion  as  measured  in bytes from the beginning of the file.  On error,
       the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
       error.

CC'ing Eric for the "official" POSIX answer....

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Max Reitz 7 years, 7 months ago 
On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
>> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
>>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
>>>> Storing the lseek() result in an int results in it overflowing when the
>>>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
>>>> being "negative" and thus thinking an error occurred when actually
>>>> everything went just fine.
>>>>
>>>> So we should use the correct type for storing the result: off_t.
>>>>
>>>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
>>>> Cc: qemu-stable@nongnu.org
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  block/file-posix.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>> index f1591c3849..90c25864a0 100644
>>>> --- a/block/file-posix.c
>>>> +++ b/block/file-posix.c
>>>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>      case PREALLOC_MODE_FULL:
>>>>      {
>>>>          int64_t num = 0, left = offset - current_length;
>>>> +        off_t seek_result;
>>>>  
>>>>          /*
>>>>           * Knowing the final size from the beginning could allow the file
>>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>  
>>>>          buf = g_malloc0(65536);
>>>>  
>>>> -        result = lseek(fd, current_length, SEEK_SET);
>>>> -        if (result < 0) {
>>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>>>> +        if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>    if (seek_result == (off_t)-1) {
>>>       ...
>>>    }
>>
>> Hmmm... On my system, it appears to be a long int[1].  And
>> find_allocation() does an off_t < 0 comparison already.  And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
> 
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
> 
>    RETURN VALUE
>        Upon successful completion, lseek() returns the resulting offset  loca‐
>        tion  as  measured  in bytes from the beginning of the file.  On error,
>        the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
>        error.
> 
> CC'ing Eric for the "official" POSIX answer....

But it also says (under NOTES):

The off_t data type is a signed integer data type specified by POSIX.1.

Max
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Daniel P. Berrangé 7 years, 7 months ago 
On Wed, Feb 28, 2018 at 02:55:22PM +0100, Max Reitz wrote:
> On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> >> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> >>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> >>>> Storing the lseek() result in an int results in it overflowing when the
> >>>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> >>>> being "negative" and thus thinking an error occurred when actually
> >>>> everything went just fine.
> >>>>
> >>>> So we should use the correct type for storing the result: off_t.
> >>>>
> >>>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >>>> Cc: qemu-stable@nongnu.org
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>> ---
> >>>>  block/file-posix.c | 5 +++--
> >>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>>> index f1591c3849..90c25864a0 100644
> >>>> --- a/block/file-posix.c
> >>>> +++ b/block/file-posix.c
> >>>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>>>      case PREALLOC_MODE_FULL:
> >>>>      {
> >>>>          int64_t num = 0, left = offset - current_length;
> >>>> +        off_t seek_result;
> >>>>  
> >>>>          /*
> >>>>           * Knowing the final size from the beginning could allow the file
> >>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>>>  
> >>>>          buf = g_malloc0(65536);
> >>>>  
> >>>> -        result = lseek(fd, current_length, SEEK_SET);
> >>>> -        if (result < 0) {
> >>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
> >>>> +        if (seek_result < 0) {
> >>>
> >>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> >>> exact value (off_t)-1 indicates an error. So this needs to be
> >>>
> >>>    if (seek_result == (off_t)-1) {
> >>>       ...
> >>>    }
> >>
> >> Hmmm... On my system, it appears to be a long int[1].  And
> >> find_allocation() does an off_t < 0 comparison already.  And
> >> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> >> types."
> > 
> > Hmm, that's odd then - lseek man page explicitly said it must be cast,
> > which suggested to me it could be unsigned:
> > 
> >    RETURN VALUE
> >        Upon successful completion, lseek() returns the resulting offset  loca‐
> >        tion  as  measured  in bytes from the beginning of the file.  On error,
> >        the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
> >        error.
> > 
> > CC'ing Eric for the "official" POSIX answer....
> 
> But it also says (under NOTES):
> 
> The off_t data type is a signed integer data type specified by POSIX.1.

Ok, lets ignore my comments then -the "< 0" vs "!= -1" difference is
harmless given this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Eric Blake 7 years, 7 months ago 
On 02/28/2018 07:53 AM, Daniel P. Berrangé wrote:

>>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>   
>>>>           buf = g_malloc0(65536);
>>>>   
>>>> -        result = lseek(fd, current_length, SEEK_SET);
>>>> -        if (result < 0) {
>>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>>>> +        if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>     if (seek_result == (off_t)-1) {
>>>        ...
>>>     }

No, off_t is ALWAYS signed, even when it is 32-bit.  The cast helps code 
that is written for 64-bit off_t to still work when compiled with the 
small file model where a 32-bit value is used (but we don't have to 
worry about that, as we always compile for large file mode with 64-bit 
off_t).

>>
>> Hmmm... On my system, it appears to be a long int[1].  And
>> find_allocation() does an off_t < 0 comparison already.  And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
> 
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
> 
>     RETURN VALUE
>         Upon successful completion, lseek() returns the resulting offset  loca‐
>         tion  as  measured  in bytes from the beginning of the file.  On error,
>         the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
>         error.
> 
> CC'ing Eric for the "official" POSIX answer....

It MAY be that the man page mentions a cast because '-1' is an int but 
'(off_t) -1' can be larger than an int.  But it may also be that you've 
uncovered something worth reporting as a bug to the man page project.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Posted by Daniel P. Berrangé 7 years, 7 months ago 
On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
> 
> So we should use the correct type for storing the result: off_t.
> 
> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>      case PREALLOC_MODE_FULL:
>      {
>          int64_t num = 0, left = offset - current_length;
> +        off_t seek_result;
>  
>          /*
>           * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>  
>          buf = g_malloc0(65536);
>  
> -        result = lseek(fd, current_length, SEEK_SET);
> -        if (result < 0) {
> +        seek_result = lseek(fd, current_length, SEEK_SET);
> +        if (seek_result < 0) {
>              result = -errno;
>              error_setg_errno(errp, -result,
>                               "Failed to seek to the old end of file");

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.