All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.