| Message ID | 20180228131315.30194-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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.
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");
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(-)