All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate
@ 2018-02-28 13:13 Max Reitz
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Max Reitz @ 2018-02-28 13:13 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Kevin Wolf, Daniel P . Berrange, qemu-stable

Fully preallocated truncation has a 50 % chance of working on images
files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
otherwise.  To make things even more interesting, often you would not
even notice because qemu reported success even though it did nothing
(because after the successful lseek(), errno was still 0, so when the
file-posix driver tried to return a negative error code, it actually
reported success).

This issue is fixed by patch 1 in this series.  Thanks to Daniel for
reporting!


Max Reitz (2):
  block/file-posix: Fix fully preallocated truncate
  iotests: Test preallocated truncate of 2G image

 block/file-posix.c         |  5 +++--
 tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/106.out | 10 ++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:13 [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Max Reitz
@ 2018-02-28 13:13 ` Max Reitz
  2018-02-28 13:34   ` Daniel P. Berrangé
  2018-02-28 13:58   ` Daniel P. Berrangé
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image Max Reitz
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Max Reitz @ 2018-02-28 13:13 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Kevin Wolf, Daniel P . Berrange, qemu-stable

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image
  2018-02-28 13:13 [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Max Reitz
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
@ 2018-02-28 13:13 ` Max Reitz
  2018-02-28 14:02   ` Daniel P. Berrangé
  2018-02-28 14:22 ` [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Eric Blake
  2018-03-26 20:30 ` Max Reitz
  3 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-02-28 13:13 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, Max Reitz, Kevin Wolf, Daniel P . Berrange, qemu-stable

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/106.out | 10 ++++++++++
 2 files changed, 34 insertions(+)

diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
index bfe71f4e60..5e51f88a78 100755
--- a/tests/qemu-iotests/106
+++ b/tests/qemu-iotests/106
@@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
     $QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode "$TEST_IMG" -${GROWTH_SIZE}K
 done
 
+echo
+echo '=== Testing image growth on 2G empty image ==='
+
+for growth_mode in falloc full; do
+    echo
+    echo "--- growth_mode=$growth_mode ---"
+
+    # Maybe we want to do an lseek() to the end of the file before the
+    # preallocation; if the file has a length of 2 GB, that would
+    # return an integer that overflows to negative when put into a
+    # plain int.  We should use the correct type for the result, and
+    # this tests we do.
+
+    _make_test_img 2G
+    $QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG" +${GROWTH_SIZE}K
+
+    actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
+    actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
+
+    if [ $actual_size -lt $GROWTH_SIZE ]; then
+        echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has ${actual_size}K"
+    fi
+done
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
index 0a42312301..c459957660 100644
--- a/tests/qemu-iotests/106.out
+++ b/tests/qemu-iotests/106.out
@@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
 
 --- growth_mode=off ---
 Image resized.
+
+=== Testing image growth on 2G empty image ===
+
+--- growth_mode=falloc ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
+
+--- growth_mode=full ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
 *** done
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
@ 2018-02-28 13:34   ` Daniel P. Berrangé
  2018-02-28 13:45     ` Max Reitz
  2018-02-28 13:58   ` Daniel P. Berrangé
  1 sibling, 1 reply; 13+ messages in thread
From: Daniel P. Berrangé @ 2018-02-28 13:34 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, qemu-devel, Kevin Wolf, qemu-stable

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 :|

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:34   ` Daniel P. Berrangé
@ 2018-02-28 13:45     ` Max Reitz
  2018-02-28 13:53       ` Daniel P. Berrangé
  0 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-02-28 13:45 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: qemu-block, qemu-devel, Kevin Wolf, qemu-stable

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:45     ` Max Reitz
@ 2018-02-28 13:53       ` Daniel P. Berrangé
  2018-02-28 13:55         ` Max Reitz
  2018-02-28 14:20         ` Eric Blake
  0 siblings, 2 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2018-02-28 13:53 UTC (permalink / raw)
  To: Max Reitz
  Cc: qemu-block, qemu-devel, eblake@redhat.com Kevin Wolf, qemu-stable

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 :|

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:53       ` Daniel P. Berrangé
@ 2018-02-28 13:55         ` Max Reitz
  2018-02-28 13:59           ` Daniel P. Berrangé
  2018-02-28 14:20         ` Eric Blake
  1 sibling, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-02-28 13:55 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: qemu-block, qemu-devel, eblake@redhat.com Kevin Wolf, qemu-stable

[-- Attachment #1: Type: text/plain, Size: 2799 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
  2018-02-28 13:34   ` Daniel P. Berrangé
@ 2018-02-28 13:58   ` Daniel P. Berrangé
  1 sibling, 0 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2018-02-28 13:58 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, qemu-devel, Kevin Wolf, qemu-stable

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 :|

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:55         ` Max Reitz
@ 2018-02-28 13:59           ` Daniel P. Berrangé
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2018-02-28 13:59 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, qemu-devel, Kevin Wolf, qemu-stable

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 :|

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image Max Reitz
@ 2018-02-28 14:02   ` Daniel P. Berrangé
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel P. Berrangé @ 2018-02-28 14:02 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, qemu-devel, Kevin Wolf, qemu-stable

On Wed, Feb 28, 2018 at 02:13:15PM +0100, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/106.out | 10 ++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
> index bfe71f4e60..5e51f88a78 100755
> --- a/tests/qemu-iotests/106
> +++ b/tests/qemu-iotests/106
> @@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
>      $QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode "$TEST_IMG" -${GROWTH_SIZE}K
>  done
>  
> +echo
> +echo '=== Testing image growth on 2G empty image ==='
> +
> +for growth_mode in falloc full; do
> +    echo
> +    echo "--- growth_mode=$growth_mode ---"
> +
> +    # Maybe we want to do an lseek() to the end of the file before the
> +    # preallocation; if the file has a length of 2 GB, that would
> +    # return an integer that overflows to negative when put into a
> +    # plain int.  We should use the correct type for the result, and
> +    # this tests we do.
> +
> +    _make_test_img 2G
> +    $QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG" +${GROWTH_SIZE}K
> +
> +    actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
> +    actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
> +
> +    if [ $actual_size -lt $GROWTH_SIZE ]; then
> +        echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has ${actual_size}K"
> +    fi
> +done
> +
>  # success, all done
>  echo '*** done'
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
> index 0a42312301..c459957660 100644
> --- a/tests/qemu-iotests/106.out
> +++ b/tests/qemu-iotests/106.out
> @@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
>  
>  --- growth_mode=off ---
>  Image resized.
> +
> +=== Testing image growth on 2G empty image ===
> +
> +--- growth_mode=falloc ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
> +
> +--- growth_mode=full ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
>  *** done

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 :|

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:53       ` Daniel P. Berrangé
  2018-02-28 13:55         ` Max Reitz
@ 2018-02-28 14:20         ` Eric Blake
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2018-02-28 14:20 UTC (permalink / raw)
  To: Daniel P. Berrangé, Max Reitz
  Cc: eblake@redhat.com Kevin Wolf, qemu-devel, qemu-block, qemu-stable

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:13 [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Max Reitz
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
  2018-02-28 13:13 ` [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image Max Reitz
@ 2018-02-28 14:22 ` Eric Blake
  2018-03-26 20:30 ` Max Reitz
  3 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2018-02-28 14:22 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-stable, qemu-devel

On 02/28/2018 07:13 AM, Max Reitz wrote:
> Fully preallocated truncation has a 50 % chance of working on images
> files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
> otherwise.  To make things even more interesting, often you would not
> even notice because qemu reported success even though it did nothing
> (because after the successful lseek(), errno was still 0, so when the
> file-posix driver tried to return a negative error code, it actually
> reported success).
> 
> This issue is fixed by patch 1 in this series.  Thanks to Daniel for
> reporting!
> 
> 
> Max Reitz (2):
>    block/file-posix: Fix fully preallocated truncate
>    iotests: Test preallocated truncate of 2G image

Good catch.  Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

gluster.c has similar code, but it assigns glfs_lseek() to an int64_t, 
so it does not have the bug.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate
  2018-02-28 13:13 [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Max Reitz
                   ` (2 preceding siblings ...)
  2018-02-28 14:22 ` [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Eric Blake
@ 2018-03-26 20:30 ` Max Reitz
  3 siblings, 0 replies; 13+ messages in thread
From: Max Reitz @ 2018-03-26 20:30 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Kevin Wolf, Daniel P . Berrange, qemu-stable

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On 2018-02-28 14:13, Max Reitz wrote:
> Fully preallocated truncation has a 50 % chance of working on images
> files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
> otherwise.  To make things even more interesting, often you would not
> even notice because qemu reported success even though it did nothing
> (because after the successful lseek(), errno was still 0, so when the
> file-posix driver tried to return a negative error code, it actually
> reported success).
> 
> This issue is fixed by patch 1 in this series.  Thanks to Daniel for
> reporting!
> 
> 
> Max Reitz (2):
>   block/file-posix: Fix fully preallocated truncate
>   iotests: Test preallocated truncate of 2G image
> 
>  block/file-posix.c         |  5 +++--
>  tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/106.out | 10 ++++++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)

Applied to my block tree...  (*cough* *cough* -- I'm the worst when it
comes to keeping track of my own patches.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-03-26 20:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-28 13:13 [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Max Reitz
2018-02-28 13:13 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
2018-02-28 13:34   ` Daniel P. Berrangé
2018-02-28 13:45     ` Max Reitz
2018-02-28 13:53       ` Daniel P. Berrangé
2018-02-28 13:55         ` Max Reitz
2018-02-28 13:59           ` Daniel P. Berrangé
2018-02-28 14:20         ` Eric Blake
2018-02-28 13:58   ` Daniel P. Berrangé
2018-02-28 13:13 ` [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image Max Reitz
2018-02-28 14:02   ` Daniel P. Berrangé
2018-02-28 14:22 ` [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate Eric Blake
2018-03-26 20:30 ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.