[PATCH 2/2] lib: vsprintf.c remove macros defining strict string functions

[PATCH 2/2] lib: vsprintf.c remove macros defining strict string functions

[Harvey Harrison]

Directly code the strict string conversion functions rather than using
defining macros.  Pull out a small helper to check the strict conditions
required at the end of a string (nul-terminated or newline).

Add additional checks in strict_strtol and strict_strtoll for numeric
overflow of the signed types.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 lib/vsprintf.c |  144 +++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 96 insertions(+), 48 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 0b39c34..ff0c93d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -128,6 +128,18 @@ long long simple_strtoll(const char *cp, char **endp, unsigned int base)
 	return simple_strtoull(cp, endp, base);
 }
 
+/*
+ * Strictly check that the string is nul terminated or has a newline
+ * immediately following len chars.
+ */
+static int strict_checktail(size_t len, const char *cp, const char *tail)
+{
+	if ((*tail == '\0') ||
+	    ((len == (size_t)(tail - cp) + 1) && (*tail == '\n')))
+		return 1;
+	else
+		return 0;
+}
 
 /**
  * strict_strtoul - convert a string to an unsigned long strictly
@@ -151,7 +163,26 @@ long long simple_strtoll(const char *cp, char **endp, unsigned int base)
  * simple_strtoul just ignores the successive invalid characters and
  * return the converted value of prefix part of the string.
  */
-int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
+int strict_strtoul(const char *cp, unsigned int base, unsigned long *res)
+{
+	char *tail;
+	unsigned long val;
+	size_t len;
+
+	*res = 0;
+	len = strlen(cp);
+	if (len == 0)
+		return -EINVAL;
+
+	val = simple_strtoul(cp, &tail, base);
+	if (strict_checktail(len, cp, tail)) {
+		*res = val;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(strict_strtoul);
 
 /**
  * strict_strtol - convert a string to a long strictly
@@ -165,7 +196,29 @@ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
  * It returns 0 if conversion is successful and *res is set to the converted
  * value, otherwise it returns -EINVAL and *res is set to 0.
  */
-int strict_strtol(const char *cp, unsigned int base, long *res);
+int strict_strtol(const char *cp, unsigned int base, long *res)
+{
+	int ret;
+	unsigned long tmp;
+
+	if (*cp == '-')
+		ret = strict_strtoul(cp + 1, base, &tmp);
+	else
+		ret = strict_strtoul(cp, base, &tmp);
+
+	if (!ret || tmp > LONG_MAX) {
+		*res = 0;
+		return -EINVAL;
+	}
+
+	if (*cp == '-')
+		*res = -tmp;
+	else
+		*res = tmp;
+
+	return 0;
+}
+EXPORT_SYMBOL(strict_strtol);
 
 /**
  * strict_strtoull - convert a string to an unsigned long long strictly
@@ -189,7 +242,26 @@ int strict_strtol(const char *cp, unsigned int base, long *res);
  * simple_strtoull just ignores the successive invalid characters and
  * return the converted value of prefix part of the string.
  */
-int strict_strtoull(const char *cp, unsigned int base, unsigned long long *res);
+int strict_strtoull(const char *cp, unsigned int base, unsigned long long *res)
+{
+	char *tail;
+	unsigned long long val;
+	size_t len;
+
+	*res = 0;
+	len = strlen(cp);
+	if (len == 0)
+		return -EINVAL;
+
+	val = simple_strtoull(cp, &tail, base);
+	if (strict_checktail(len, cp, tail)) {
+		*res = val;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(strict_strtoull);
 
 /**
  * strict_strtoll - convert a string to a long long strictly
@@ -203,53 +275,29 @@ int strict_strtoull(const char *cp, unsigned int base, unsigned long long *res);
  * It returns 0 if conversion is successful and *res is set to the converted
  * value, otherwise it returns -EINVAL and *res is set to 0.
  */
-int strict_strtoll(const char *cp, unsigned int base, long long *res);
-
-#define define_strict_strtoux(type, valtype)				\
-int strict_strtou##type(const char *cp, unsigned int base, valtype *res)\
-{									\
-	char *tail;							\
-	valtype val;							\
-	size_t len;							\
-									\
-	*res = 0;							\
-	len = strlen(cp);						\
-	if (len == 0)							\
-		return -EINVAL;						\
-									\
-	val = simple_strtoul(cp, &tail, base);				\
-	if ((*tail == '\0') ||						\
-		((len == (size_t)(tail - cp) + 1) && (*tail == '\n'))) {\
-		*res = val;						\
-		return 0;						\
-	}								\
-									\
-	return -EINVAL;							\
-}									\
-
-#define define_strict_strtox(type, valtype)				\
-int strict_strto##type(const char *cp, unsigned int base, valtype *res)	\
-{									\
-	int ret;							\
-	if (*cp == '-') {						\
-		ret = strict_strtou##type(cp+1, base, res);		\
-		if (!ret)						\
-			*res = -(*res);					\
-	} else								\
-		ret = strict_strtou##type(cp, base, res);		\
-									\
-	return ret;							\
-}									\
-
-define_strict_strtoux(l, unsigned long)
-define_strict_strtox(l, long)
-define_strict_strtoux(ll, unsigned long long)
-define_strict_strtox(ll, long long)
+int strict_strtoll(const char *cp, unsigned int base, long long *res)
+{
+	int ret;
+	unsigned long long tmp;
 
-EXPORT_SYMBOL(strict_strtoul);
-EXPORT_SYMBOL(strict_strtol);
+	if (*cp == '-')
+		ret = strict_strtoull(cp + 1, base, &tmp);
+	else
+		ret = strict_strtoull(cp, base, &tmp);
+
+	if (!ret || tmp > LLONG_MAX) {
+		*res = 0;
+		return -EINVAL;
+	}
+
+	if (*cp == '-')
+		*res = -tmp;
+	else
+		*res = tmp;
+
+	return 0;
+}
 EXPORT_SYMBOL(strict_strtoll);
-EXPORT_SYMBOL(strict_strtoull);
 
 static int skip_atoi(const char **s)
 {
-- 
1.5.5.1.350.gbbbf

Re: [PATCH 2/2] lib: vsprintf.c remove macros defining strict string functions

[Harvey Harrison]

On Thu, 2008-05-08 at 00:09 +0400, Alexey Dobriyan wrote:
> On Wed, May 07, 2008 at 11:25:29AM -0700, Harvey Harrison wrote:
> > Directly code the strict string conversion functions rather than using
> > defining macros.  Pull out a small helper to check the strict conditions
> > required at the end of a string (nul-terminated or newline).
> 
> > Add additional checks in strict_strtol and strict_strtoll for numeric
> > overflow of the signed types.
> 
> C interer ranges are asymmetric.

Yes, and LLONG_MAX = -LLONG_MIN + 1...so it will reject string values
of LLONG_MIN...easily fixed if we want it I guess.

> 
> These "strict_" functions are a farce. No amount of afterchecking will
> save you if there is trivial wraparound in core function.
> 

That's also true, I guess it depends on how far we want to go.  And
after that point, just stick a WARN_ON and return -EINVAL.

I'm not sure where the balance is, but I think my patch is still on
the useful side of it.

> 	Alexey "kstrtonum" Dobriyan
> 
> > + */
> > +static int strict_checktail(size_t len, const char *cp, const char *tail)
> 
> Name simply sucks.

Sure does, care to suggest something better, at least it had a
comment ;-)

Anyways, thanks for taking a look.

Harvey

Re: [PATCH 2/2] lib: vsprintf.c remove macros defining strict string functions

[Harvey Harrison]

On Wed, 2008-05-07 at 12:20 -0700, Harvey Harrison wrote:
> On Thu, 2008-05-08 at 00:09 +0400, Alexey Dobriyan wrote:
> > On Wed, May 07, 2008 at 11:25:29AM -0700, Harvey Harrison wrote:
> > > Directly code the strict string conversion functions rather than using
> > > defining macros.  Pull out a small helper to check the strict conditions
> > > required at the end of a string (nul-terminated or newline).
> > 
> > > Add additional checks in strict_strtol and strict_strtoll for numeric
> > > overflow of the signed types.
> > 
> > C interer ranges are asymmetric.
> 
> Yes, and LLONG_MAX = -LLONG_MIN + 1...so it will reject string values
> of LLONG_MIN...easily fixed if we want it I guess.


Missed the braces above obviously....but the point stands.

Harvey

Re: [PATCH 2/2] lib: vsprintf.c remove macros defining strict string functions

[Alexey Dobriyan]

On Wed, May 07, 2008 at 11:25:29AM -0700, Harvey Harrison wrote:
> Directly code the strict string conversion functions rather than using
> defining macros.  Pull out a small helper to check the strict conditions
> required at the end of a string (nul-terminated or newline).

> Add additional checks in strict_strtol and strict_strtoll for numeric
> overflow of the signed types.

C interer ranges are asymmetric.

These "strict_" functions are a farce. No amount of afterchecking will
save you if there is trivial wraparound in core function.

	Alexey "kstrtonum" Dobriyan


> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -128,6 +128,18 @@ long long simple_strtoll(const char *cp, char **endp, unsigned int base)
>  	return simple_strtoull(cp, endp, base);
>  }
>  
> +/*
> + * Strictly check that the string is nul terminated or has a newline
> + * immediately following len chars.
> + */
> +static int strict_checktail(size_t len, const char *cp, const char *tail)

Name simply sucks.

> +{
> +	if ((*tail == '\0') ||
> +	    ((len == (size_t)(tail - cp) + 1) && (*tail == '\n')))
> +		return 1;
> +	else
> +		return 0;
> +}

> @@ -165,7 +196,29 @@ int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
>   * It returns 0 if conversion is successful and *res is set to the converted
>   * value, otherwise it returns -EINVAL and *res is set to 0.
>   */
> -int strict_strtol(const char *cp, unsigned int base, long *res);
> +int strict_strtol(const char *cp, unsigned int base, long *res)
> +{
> +	int ret;
> +	unsigned long tmp;
> +
> +	if (*cp == '-')
> +		ret = strict_strtoul(cp + 1, base, &tmp);
> +	else
> +		ret = strict_strtoul(cp, base, &tmp);
> +
> +	if (!ret || tmp > LONG_MAX) {
> +		*res = 0;
> +		return -EINVAL;
> +	}
> +
> +	if (*cp == '-')
> +		*res = -tmp;
> +	else
> +		*res = tmp;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(strict_strtol);
>  
>  /**
>   * strict_strtoull - convert a string to an unsigned long long strictly