* [PATCH] Catch under/overflow cases in cvtnum() and cvttime().
@ 2014-07-11 19:34 Arkadiusz Miśkiewicz
2014-07-11 23:14 ` Eric Sandeen
2014-07-13 17:38 ` [PATCH] Init errno before strto* calls Arkadiusz Miśkiewicz
0 siblings, 2 replies; 12+ messages in thread
From: Arkadiusz Miśkiewicz @ 2014-07-11 19:34 UTC (permalink / raw)
To: xfs
cvtnum() and cvttime() silently ignore overflows. This leads to error
conditions not being catched. Example:
$ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
bhard=987654321098765432199 999' /
$
Fixed version:
$ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
bhard=987654321098765432199 999' /
xfs_quota: Error: could not parse size 987654321098765432199.
xfs_quota: unrecognised argument bsoft=987654321098765432199
Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
---
libxcmd/input.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/libxcmd/input.c b/libxcmd/input.c
index c06b5b8..397a124 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -154,6 +154,8 @@ cvtnum(
int c;
i = strtoll(s, &sp, 0);
+ if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
+ return -1LL;
if (i == 0 && sp == s)
return -1LL;
if (*sp == '\0')
@@ -238,6 +240,8 @@ cvttime(
char *sp;
i = strtoul(s, &sp, 0);
+ if (i == ULONG_MAX && errno == ERANGE)
+ return 0;
if (i == 0 && sp == s)
return 0;
if (*sp == '\0')
--
2.0.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Catch under/overflow cases in cvtnum() and cvttime().
2014-07-11 19:34 [PATCH] Catch under/overflow cases in cvtnum() and cvttime() Arkadiusz Miśkiewicz
@ 2014-07-11 23:14 ` Eric Sandeen
2014-07-12 6:13 ` Arkadiusz Miśkiewicz
2014-07-13 17:38 ` [PATCH] Init errno before strto* calls Arkadiusz Miśkiewicz
1 sibling, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2014-07-11 23:14 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz, xfs
On 7/11/14, 2:34 PM, Arkadiusz Miśkiewicz wrote:
> cvtnum() and cvttime() silently ignore overflows. This leads to error
> conditions not being catched. Example:
>
> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
> bhard=987654321098765432199 999' /
> $
>
> Fixed version:
> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
> bhard=987654321098765432199 999' /
> xfs_quota: Error: could not parse size 987654321098765432199.
> xfs_quota: unrecognised argument bsoft=987654321098765432199
So, strtol(3) suggests setting errno to 0 before the call:
NOTES
Since strtol() can legitimately return 0, LONG_MAX, or LONG_MIN
(LLONG_MAX or LLONG_MIN for strtoll()) on both success and failure, the
calling program should set errno to 0 before the call, and then deter-
mine if an error occurred by checking whether errno has a non-zero
value after the call.
Ditto for strtoul().
I guess that is just to ensure that there's not a leftover errno
when we make the call? Worth doing, maybe?
Thanks,
-Eric
> Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
> ---
> libxcmd/input.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/libxcmd/input.c b/libxcmd/input.c
> index c06b5b8..397a124 100644
> --- a/libxcmd/input.c
> +++ b/libxcmd/input.c
> @@ -154,6 +154,8 @@ cvtnum(
> int c;
>
> i = strtoll(s, &sp, 0);
> + if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
> + return -1LL;
> if (i == 0 && sp == s)
> return -1LL;
> if (*sp == '\0')
> @@ -238,6 +240,8 @@ cvttime(
> char *sp;
>
> i = strtoul(s, &sp, 0);
> + if (i == ULONG_MAX && errno == ERANGE)
> + return 0;
> if (i == 0 && sp == s)
> return 0;
> if (*sp == '\0')
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Catch under/overflow cases in cvtnum() and cvttime().
2014-07-11 23:14 ` Eric Sandeen
@ 2014-07-12 6:13 ` Arkadiusz Miśkiewicz
2014-07-12 13:37 ` Eric Sandeen
0 siblings, 1 reply; 12+ messages in thread
From: Arkadiusz Miśkiewicz @ 2014-07-12 6:13 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Saturday 12 of July 2014, Eric Sandeen wrote:
> On 7/11/14, 2:34 PM, Arkadiusz Miśkiewicz wrote:
> > cvtnum() and cvttime() silently ignore overflows. This leads to error
> > conditions not being catched. Example:
> >
> > $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
> >
> > bhard=987654321098765432199 999' /
> >
> > $
> >
> > Fixed version:
> > $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
> >
> > bhard=987654321098765432199 999' /
> >
> > xfs_quota: Error: could not parse size 987654321098765432199.
> > xfs_quota: unrecognised argument bsoft=987654321098765432199
>
> So, strtol(3) suggests setting errno to 0 before the call:
>
> NOTES
> Since strtol() can legitimately return 0, LONG_MAX, or
> LONG_MIN (LLONG_MAX or LLONG_MIN for strtoll()) on both success and
> failure, the calling program should set errno to 0 before the call, and
> then deter- mine if an error occurred by checking whether errno has a
> non-zero value after the call.
>
> Ditto for strtoul().
Hm, my man pages 3.70 don't have such notes, strtol(3):
NOTES
In locales other than the "C" locale, also other strings may be
accepted. (For example, the thousands separator of the current locale may be
supported.)
BSD also has
quad_t
strtoq(const char *nptr, char **endptr, int base);
with completely analogous definition. Depending on the wordsize of the
current architecture, this may be equivalent to strtoll() or to strtol().
>
> I guess that is just to ensure that there's not a leftover errno
> when we make the call? Worth doing, maybe?
ERANGE is checked in few other places already in input.c and none initialize
errno before strtoul() call.
>
> Thanks,
> -Eric
>
> > Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
> > ---
> >
> > libxcmd/input.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/libxcmd/input.c b/libxcmd/input.c
> > index c06b5b8..397a124 100644
> > --- a/libxcmd/input.c
> > +++ b/libxcmd/input.c
> > @@ -154,6 +154,8 @@ cvtnum(
> >
> > int c;
> >
> > i = strtoll(s, &sp, 0);
> >
> > + if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
> > + return -1LL;
> >
> > if (i == 0 && sp == s)
> >
> > return -1LL;
> >
> > if (*sp == '\0')
> >
> > @@ -238,6 +240,8 @@ cvttime(
> >
> > char *sp;
> >
> > i = strtoul(s, &sp, 0);
> >
> > + if (i == ULONG_MAX && errno == ERANGE)
> > + return 0;
> >
> > if (i == 0 && sp == s)
> >
> > return 0;
> >
> > if (*sp == '\0')
--
Arkadiusz Miśkiewicz, arekm / maven.pl
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Catch under/overflow cases in cvtnum() and cvttime().
2014-07-12 6:13 ` Arkadiusz Miśkiewicz
@ 2014-07-12 13:37 ` Eric Sandeen
2014-07-12 13:43 ` Eric Sandeen
0 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2014-07-12 13:37 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On 7/12/14, 1:13 AM, Arkadiusz Miśkiewicz wrote:
> On Saturday 12 of July 2014, Eric Sandeen wrote:
>> On 7/11/14, 2:34 PM, Arkadiusz Miśkiewicz wrote:
>>> cvtnum() and cvttime() silently ignore overflows. This leads to error
>>> conditions not being catched. Example:
>>>
>>> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
>>>
>>> bhard=987654321098765432199 999' /
>>>
>>> $
>>>
>>> Fixed version:
>>> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
>>>
>>> bhard=987654321098765432199 999' /
>>>
>>> xfs_quota: Error: could not parse size 987654321098765432199.
>>> xfs_quota: unrecognised argument bsoft=987654321098765432199
>>
>> So, strtol(3) suggests setting errno to 0 before the call:
>>
>> NOTES
>> Since strtol() can legitimately return 0, LONG_MAX, or
>> LONG_MIN (LLONG_MAX or LLONG_MIN for strtoll()) on both success and
>> failure, the calling program should set errno to 0 before the call, and
>> then deter- mine if an error occurred by checking whether errno has a
>> non-zero value after the call.
>>
>> Ditto for strtoul().
>
> Hm, my man pages 3.70 don't have such notes, strtol(3):
>
> NOTES
> In locales other than the "C" locale, also other strings may be
> accepted. (For example, the thousands separator of the current locale may be
> supported.)
>
> BSD also has
>
> quad_t
> strtoq(const char *nptr, char **endptr, int base);
>
> with completely analogous definition. Depending on the wordsize of the
> current architecture, this may be equivalent to strtoll() or to strtol().
>
>>
>> I guess that is just to ensure that there's not a leftover errno
>> when we make the call? Worth doing, maybe?
>
> ERANGE is checked in few other places already in input.c and none initialize
> errno before strtoul() call.
http://c-faq.com/misc/errno.html suggests it too:
> It's only necessary to detect errors with errno when a function does
> not have a unique, unambiguous, out-of-band error return (i.e.
> because all of its possible return values are valid; one example is
> atoi). In these cases (and in these cases only; check the
> documentation to be sure whether a function allows this), you can
> detect errors by setting errno to 0, calling the function, then
> testing errno.
I wonder why it was removed from the man page, it makes sense to me, but
maybe I'm missing something.
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Catch under/overflow cases in cvtnum() and cvttime().
2014-07-12 13:37 ` Eric Sandeen
@ 2014-07-12 13:43 ` Eric Sandeen
0 siblings, 0 replies; 12+ messages in thread
From: Eric Sandeen @ 2014-07-12 13:43 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On 7/12/14, 8:37 AM, Eric Sandeen wrote:
> On 7/12/14, 1:13 AM, Arkadiusz Miśkiewicz wrote:
>> On Saturday 12 of July 2014, Eric Sandeen wrote:
>>> On 7/11/14, 2:34 PM, Arkadiusz Miśkiewicz wrote:
>>>> cvtnum() and cvttime() silently ignore overflows. This leads to error
>>>> conditions not being catched. Example:
>>>>
>>>> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
>>>>
>>>> bhard=987654321098765432199 999' /
>>>>
>>>> $
>>>>
>>>> Fixed version:
>>>> $ xfs_quota -x -c 'limit -u bsoft=987654321098765432199 \
>>>>
>>>> bhard=987654321098765432199 999' /
>>>>
>>>> xfs_quota: Error: could not parse size 987654321098765432199.
>>>> xfs_quota: unrecognised argument bsoft=987654321098765432199
>>>
>>> So, strtol(3) suggests setting errno to 0 before the call:
>>>
>>> NOTES
>>> Since strtol() can legitimately return 0, LONG_MAX, or
>>> LONG_MIN (LLONG_MAX or LLONG_MIN for strtoll()) on both success and
>>> failure, the calling program should set errno to 0 before the call, and
>>> then deter- mine if an error occurred by checking whether errno has a
>>> non-zero value after the call.
>>>
>>> Ditto for strtoul().
>>
>> Hm, my man pages 3.70 don't have such notes, strtol(3):
>>
>> NOTES
>> In locales other than the "C" locale, also other strings may be
>> accepted. (For example, the thousands separator of the current locale may be
>> supported.)
>>
>> BSD also has
>>
>> quad_t
>> strtoq(const char *nptr, char **endptr, int base);
>>
>> with completely analogous definition. Depending on the wordsize of the
>> current architecture, this may be equivalent to strtoll() or to strtol().
>>
>>>
>>> I guess that is just to ensure that there's not a leftover errno
>>> when we make the call? Worth doing, maybe?
>>
>> ERANGE is checked in few other places already in input.c and none initialize
>> errno before strtoul() call.
>
> http://c-faq.com/misc/errno.html suggests it too:
>
>> It's only necessary to detect errors with errno when a function does
>> not have a unique, unambiguous, out-of-band error return (i.e.
>> because all of its possible return values are valid; one example is
>> atoi). In these cases (and in these cases only; check the
>> documentation to be sure whether a function allows this), you can
>> detect errors by setting errno to 0, calling the function, then
>> testing errno.
>
> I wonder why it was removed from the man page
Actually it seems to still be there:
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/tree/man3/strtol.3#n190
fiddly detail but probably worth doing...
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Init errno before strto* calls.
2014-07-11 19:34 [PATCH] Catch under/overflow cases in cvtnum() and cvttime() Arkadiusz Miśkiewicz
2014-07-11 23:14 ` Eric Sandeen
@ 2014-07-13 17:38 ` Arkadiusz Miśkiewicz
2014-07-13 23:04 ` Dave Chinner
1 sibling, 1 reply; 12+ messages in thread
From: Arkadiusz Miśkiewicz @ 2014-07-13 17:38 UTC (permalink / raw)
To: xfs
Eric Sandeen noted that strtol(3) and friends require errno
initialization. From (fresh) man page:
NOTES
Since strtol() can legitimately return 0, LONG_MAX,
or LONG_MIN (LLONG_MAX or LLONG_MIN for strtoll()) on both
success and failure, the calling program should set errno
to 0 before the call, and then determine if an error
occurred by checking whether errno has a non-zero
value after the call.
So do it.
---
libxcmd/input.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/libxcmd/input.c b/libxcmd/input.c
index 397a124..711b527 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -153,6 +153,7 @@ cvtnum(
char *sp;
int c;
+ errno = 0;
i = strtoll(s, &sp, 0);
if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
return -1LL;
@@ -239,6 +240,7 @@ cvttime(
unsigned long i;
char *sp;
+ errno = 0;
i = strtoul(s, &sp, 0);
if (i == ULONG_MAX && errno == ERANGE)
return 0;
@@ -348,6 +350,7 @@ prid_from_string(
* Allow either a full numeric or a valid projectname, even
* if it starts with a digit.
*/
+ errno = 0;
prid_long = strtoul(project, &sp, 10);
if (*project != '\0' && *sp == '\0') {
if ((prid_long == ULONG_MAX && errno == ERANGE)
@@ -369,6 +372,7 @@ uid_from_string(
unsigned long uid_long;
char *sp;
+ errno = 0;
uid_long = strtoul(user, &sp, 10);
if (sp != user) {
if ((uid_long == ULONG_MAX && errno == ERANGE)
@@ -390,6 +394,7 @@ gid_from_string(
unsigned long gid_long;
char *sp;
+ errno = 0;
gid_long = strtoul(group, &sp, 10);
if (sp != group) {
if ((gid_long == ULONG_MAX && errno == ERANGE)
--
2.0.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Init errno before strto* calls.
2014-07-13 17:38 ` [PATCH] Init errno before strto* calls Arkadiusz Miśkiewicz
@ 2014-07-13 23:04 ` Dave Chinner
2014-07-14 7:56 ` [PATCH] Detect strto* failures based on errno Arkadiusz Miśkiewicz
0 siblings, 1 reply; 12+ messages in thread
From: Dave Chinner @ 2014-07-13 23:04 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On Sun, Jul 13, 2014 at 07:38:08PM +0200, Arkadiusz Miśkiewicz wrote:
> Eric Sandeen noted that strtol(3) and friends require errno
> initialization. From (fresh) man page:
>
> NOTES
> Since strtol() can legitimately return 0, LONG_MAX,
> or LONG_MIN (LLONG_MAX or LLONG_MIN for strtoll()) on both
> success and failure, the calling program should set errno
> to 0 before the call, and then determine if an error
> occurred by checking whether errno has a non-zero
> value after the call.
>
> So do it.
> ---
> libxcmd/input.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/libxcmd/input.c b/libxcmd/input.c
> index 397a124..711b527 100644
> --- a/libxcmd/input.c
> +++ b/libxcmd/input.c
> @@ -153,6 +153,7 @@ cvtnum(
> char *sp;
> int c;
>
> + errno = 0;
> i = strtoll(s, &sp, 0);
> if ((i == LLONG_MIN || i == LLONG_MAX) && errno == ERANGE)
> return -1LL;
I think that just checking for (errno != 0) is better here, because
then we also catch errors from unsupported formats or invalid
strings (i.e. EINVAL).
i.e. if the result is out of range, then ERANGE is always returned,
so we don't need to check the actual value at all...
Both patches could be condensed down to a single patch that does:
+ errno = 0;
i = strtoll(s, &sp, 0);
+ if (errno)
+ return -1LL;
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Detect strto* failures based on errno.
2014-07-13 23:04 ` Dave Chinner
@ 2014-07-14 7:56 ` Arkadiusz Miśkiewicz
2014-07-16 0:00 ` Dave Chinner
2014-07-16 3:01 ` Dave Chinner
0 siblings, 2 replies; 12+ messages in thread
From: Arkadiusz Miśkiewicz @ 2014-07-14 7:56 UTC (permalink / raw)
To: xfs
Code was testing for ERANGE errno only in some places. In other places
it didn't do any errno checking at all.
Unify strto* result testing by treating any non zero errno as failure.
Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
---
libxcmd/input.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/libxcmd/input.c b/libxcmd/input.c
index c06b5b8..d72dff3 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -153,8 +153,9 @@ cvtnum(
char *sp;
int c;
+ errno = 0;
i = strtoll(s, &sp, 0);
- if (i == 0 && sp == s)
+ if (errno || (i == 0 && sp == s))
return -1LL;
if (*sp == '\0')
return i;
@@ -237,8 +238,9 @@ cvttime(
unsigned long i;
char *sp;
+ errno = 0;
i = strtoul(s, &sp, 0);
- if (i == 0 && sp == s)
+ if (errno || (i == 0 && sp == s))
return 0;
if (*sp == '\0')
return i;
@@ -344,10 +346,10 @@ prid_from_string(
* Allow either a full numeric or a valid projectname, even
* if it starts with a digit.
*/
+ errno = 0;
prid_long = strtoul(project, &sp, 10);
if (*project != '\0' && *sp == '\0') {
- if ((prid_long == ULONG_MAX && errno == ERANGE)
- || (prid_long > (prid_t)-1))
+ if (errno || (prid_long > (prid_t)-1))
return -1;
return (prid_t)prid_long;
}
@@ -365,10 +367,10 @@ uid_from_string(
unsigned long uid_long;
char *sp;
+ errno = 0;
uid_long = strtoul(user, &sp, 10);
if (sp != user) {
- if ((uid_long == ULONG_MAX && errno == ERANGE)
- || (uid_long > (uid_t)-1))
+ if (errno || (uid_long > (uid_t)-1))
return -1;
return (uid_t)uid_long;
}
@@ -386,10 +388,10 @@ gid_from_string(
unsigned long gid_long;
char *sp;
+ errno = 0;
gid_long = strtoul(group, &sp, 10);
if (sp != group) {
- if ((gid_long == ULONG_MAX && errno == ERANGE)
- || (gid_long > (gid_t)-1))
+ if (errno || (gid_long > (gid_t)-1))
return -1;
return (gid_t)gid_long;
}
--
2.0.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Detect strto* failures based on errno.
2014-07-14 7:56 ` [PATCH] Detect strto* failures based on errno Arkadiusz Miśkiewicz
@ 2014-07-16 0:00 ` Dave Chinner
2014-07-16 3:01 ` Dave Chinner
1 sibling, 0 replies; 12+ messages in thread
From: Dave Chinner @ 2014-07-16 0:00 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On Mon, Jul 14, 2014 at 09:56:59AM +0200, Arkadiusz Miśkiewicz wrote:
> Code was testing for ERANGE errno only in some places. In other places
> it didn't do any errno checking at all.
>
> Unify strto* result testing by treating any non zero errno as failure.
>
> Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
Arkadiusz, why did git-send-email encode this entire commit in
base64? Can you configure git to send patches in plain text? even
UTF-8 encoding is fine.
Yes, I know git am handles base64 encoded emails, but humans
don't.... :/
As it is, the patch is fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Detect strto* failures based on errno.
2014-07-14 7:56 ` [PATCH] Detect strto* failures based on errno Arkadiusz Miśkiewicz
2014-07-16 0:00 ` Dave Chinner
@ 2014-07-16 3:01 ` Dave Chinner
2014-07-16 7:44 ` Arkadiusz Miśkiewicz
1 sibling, 1 reply; 12+ messages in thread
From: Dave Chinner @ 2014-07-16 3:01 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On Mon, Jul 14, 2014 at 09:56:59AM +0200, Arkadiusz Miśkiewicz wrote:
> Code was testing for ERANGE errno only in some places. In other places
> it didn't do any errno checking at all.
>
> Unify strto* result testing by treating any non zero errno as failure.
>
> Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
This patch appears to cause xfs/071 to fail:
Writing 512 bytes, offset is +0 (direct=false)
-pwrite64: File too large
+non-numeric offset argument -- <OFFSET>
Reading 512 bytes (direct=false)
read 0/512 bytes at offset <OFFSET>
...
(Run 'diff -u tests/xfs/071.out /home/dave/src/xfstests-dev/results//xfs/071.out.bad' to see the entire diff)
Can you have a look into this?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Detect strto* failures based on errno.
2014-07-16 3:01 ` Dave Chinner
@ 2014-07-16 7:44 ` Arkadiusz Miśkiewicz
2014-07-16 23:32 ` Dave Chinner
0 siblings, 1 reply; 12+ messages in thread
From: Arkadiusz Miśkiewicz @ 2014-07-16 7:44 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Wednesday 16 of July 2014, Dave Chinner wrote:
> On Mon, Jul 14, 2014 at 09:56:59AM +0200, Arkadiusz Miśkiewicz wrote:
> > Code was testing for ERANGE errno only in some places. In other places
> > it didn't do any errno checking at all.
> >
> > Unify strto* result testing by treating any non zero errno as failure.
> >
> > Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
>
> This patch appears to cause xfs/071 to fail:
>
>
>
> Writing 512 bytes, offset is +0 (direct=false)
> -pwrite64: File too large
> +non-numeric offset argument -- <OFFSET>
> Reading 512 bytes (direct=false)
> read 0/512 bytes at offset <OFFSET>
> ...
> (Run 'diff -u tests/xfs/071.out
> /home/dave/src/xfstests-dev/results//xfs/071.out.bad' to see the entire
> diff)
>
> Can you have a look into this?
The test runs:
xfs_io -c 'pwrite 9223373136366403583 512' file
where 9223373136366403583 is bigger than LLONG_MAX (9223372036854775807), so
# LC_ALL=C xfs_io -c 'pwrite 9223373136366403583 512' ./x
(MYDEBUG)strerror(34): Numerical result out of range
non-numeric offset argument -- 9223373136366403583
What would be best approach to fix this - some cvtunum for unsigned long long?
> Cheers,
>
> Dave.
--
Arkadiusz Miśkiewicz, arekm / maven.pl
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Detect strto* failures based on errno.
2014-07-16 7:44 ` Arkadiusz Miśkiewicz
@ 2014-07-16 23:32 ` Dave Chinner
0 siblings, 0 replies; 12+ messages in thread
From: Dave Chinner @ 2014-07-16 23:32 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: xfs
On Wed, Jul 16, 2014 at 09:44:07AM +0200, Arkadiusz Miśkiewicz wrote:
> On Wednesday 16 of July 2014, Dave Chinner wrote:
> > On Mon, Jul 14, 2014 at 09:56:59AM +0200, Arkadiusz Miśkiewicz wrote:
> > > Code was testing for ERANGE errno only in some places. In other places
> > > it didn't do any errno checking at all.
> > >
> > > Unify strto* result testing by treating any non zero errno as failure.
> > >
> > > Signed-off-by: Arkadiusz Miśkiewicz <arekm@maven.pl>
> >
> > This patch appears to cause xfs/071 to fail:
> >
> >
> >
> > Writing 512 bytes, offset is +0 (direct=false)
> > -pwrite64: File too large
> > +non-numeric offset argument -- <OFFSET>
> > Reading 512 bytes (direct=false)
> > read 0/512 bytes at offset <OFFSET>
> > ...
> > (Run 'diff -u tests/xfs/071.out
> > /home/dave/src/xfstests-dev/results//xfs/071.out.bad' to see the entire
> > diff)
> >
> > Can you have a look into this?
>
> The test runs:
> xfs_io -c 'pwrite 9223373136366403583 512' file
>
> where 9223373136366403583 is bigger than LLONG_MAX (9223372036854775807), so
>
> # LC_ALL=C xfs_io -c 'pwrite 9223373136366403583 512' ./x
> (MYDEBUG)strerror(34): Numerical result out of range
> non-numeric offset argument -- 9223373136366403583
>
>
> What would be best approach to fix this - some cvtunum for unsigned long long?
I'm pretty sure that cvtnum is only supposed to work with positive
integers - it returns negative values as an error. hence just
changing to use strtoull() would probably fix the issue.
The caller can determine if a negative number is valid or not....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-07-16 23:32 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-11 19:34 [PATCH] Catch under/overflow cases in cvtnum() and cvttime() Arkadiusz Miśkiewicz
2014-07-11 23:14 ` Eric Sandeen
2014-07-12 6:13 ` Arkadiusz Miśkiewicz
2014-07-12 13:37 ` Eric Sandeen
2014-07-12 13:43 ` Eric Sandeen
2014-07-13 17:38 ` [PATCH] Init errno before strto* calls Arkadiusz Miśkiewicz
2014-07-13 23:04 ` Dave Chinner
2014-07-14 7:56 ` [PATCH] Detect strto* failures based on errno Arkadiusz Miśkiewicz
2014-07-16 0:00 ` Dave Chinner
2014-07-16 3:01 ` Dave Chinner
2014-07-16 7:44 ` Arkadiusz Miśkiewicz
2014-07-16 23:32 ` Dave Chinner
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.