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

[Daniel P . Berrangé]

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 <address@hidden>
> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >>>> Cc: address@hidden
> >>>> Signed-off-by: Max Reitz <address@hidden>
> >>>> ---
> >>>>  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 :|