Re: [PATCH] fix overflow handling in dd(1)

[Ingo Schwarze]

Hi,

Todd C. Miller wrote on Fri, Sep 12, 2014 at 09:23:31AM -0600:
 On Thu, 11 Sep 2014 22:03:04 -0700, William Orr wrote:

 I'm resubmitting this patch since the source tree was locked last time I
 submitted. Any thoughts?

 I think we've discussed this one to death already.  It looks fine
 to me.

Survived my testing, too, so i have put it in.

Thanks for perservering,
  Ingo

Re: [PATCH] fix overflow handling in dd(1)

[Todd C. Miller]

On Thu, 11 Sep 2014 22:03:04 -0700, William Orr wrote:

 I'm resubmitting this patch since the source tree was locked last time I
 submitted. Any thoughts?

I think we've discussed this one to death already.  It looks fine
to me.

 - todd

[PATCH] fix overflow handling in dd(1)

[William Orr]

Hey,

I'm resubmitting this patch since the source tree was locked last time I
submitted. Any thoughts?

Thanks,
William Orr

Index: bin/dd/args.c
===
RCS file: /cvs/src/bin/dd/args.c,v
retrieving revision 1.25
diff -u -b -w -p -r1.25 args.c
--- bin/dd/args.c   21 May 2014 06:23:02 -  1.25
+++ bin/dd/args.c   12 Sep 2014 04:51:07 -
@@ -323,8 +323,12 @@ get_bsz(char *val)
size_t num, t;
char *expr;
 
+   if (strchr(val, '-'))
+   errx(1, %s: illegal numeric value, oper);
+
+   errno = 0;
num = strtoul(val, expr, 0);
-   if (num == SIZE_T_MAX)  /* Overflow. */
+   if (num == ULONG_MAX  errno == ERANGE)/* Overflow. */
err(1, %s, oper);
if (expr == val)/* No digits. */
errx(1, %s: illegal numeric value, oper);

Re: [PATCH] fix overflow handling in dd(1)

[William Orr]

Hey,

Sorry to bring this up again, but are there any other changes that need
to be made to this patch? I've fixed all of the major complaints.

Thanks,
William Orr

On 07/13/2014 02:19 AM, William Orr wrote:
 Here is the latest diff with the bullshit removed and the loop replaced
 with strchr.
 
 Index: bin/dd/args.c
 ===
 RCS file: /cvs/src/bin/dd/args.c,v
 retrieving revision 1.25
 diff -u -b -w -p -r1.25 args.c
 --- bin/dd/args.c21 May 2014 06:23:02 -1.25
 +++ bin/dd/args.c13 Jul 2014 09:13:18 -
 @@ -196,8 +196,7 @@ static void
  f_count(char *arg)
  {
  
 -if ((cpy_cnt = get_bsz(arg)) == 0)
 -cpy_cnt = (size_t)-1;
 +cpy_cnt = get_bsz(arg);
  }
  
  static void
 @@ -323,8 +322,12 @@ get_bsz(char *val)
  size_t num, t;
  char *expr;
  
 -num = strtoul(val, expr, 0);
 -if (num == SIZE_T_MAX)/* Overflow. */
 +if (strchr(val, '-'))
 +errx(1, %s: illegal numeric value, oper);
 +
 +errno = 0;
 +num = strtoul(val, expr, 0);
 +if (num == ULONG_MAX  errno == ERANGE)/* Overflow. */
  err(1, %s, oper);
  if (expr == val)/* No digits. */
  errx(1, %s: illegal numeric value, oper);
 Index: bin/dd/dd.c
 ===
 RCS file: /cvs/src/bin/dd/dd.c,v
 retrieving revision 1.18
 diff -u -b -w -p -r1.18 dd.c
 --- bin/dd/dd.c1 Jun 2013 16:46:49 -1.18
 +++ bin/dd/dd.c13 Jul 2014 09:13:18 -
 @@ -77,7 +77,7 @@ main(int argc, char *argv[])
  
  atexit(summary);
  
 -if (cpy_cnt != (size_t)-1) {
 +if (cpy_cnt != 0) {
  while (files_cnt--)
  dd_in();
  }
 
 On 7/13/2014 2:08 AM, William Orr wrote:
 Sorry, the libssl patch was unintentional. I forgot to cvs up -C that
 one.

 On 7/13/2014 2:05 AM, Ted Unangst wrote:
 On Sun, Jul 13, 2014 at 01:52, William Orr wrote:
 Hey,

 I sent a patch similar to this almost a month ago with no response.

 Feedback? Interest?
 Yes.

 -num = strtoul(val, expr, 0);
 -if (num == SIZE_T_MAX)/* Overflow. */
 +while (isspace(vp[0]))
 +vp++;
 +if (vp[0] == '-')
 +errx(1, %s: cannot be negative, oper);
 +
 +errno = 0;
 +num = strtoul(vp, expr, 0);
 +if (num == SIZE_T_MAX  errno == ERANGE) /* Overflow. */
 I think you can just use strchr to look for a - anywhere in the
 string. It shouldn't be anywhere, right? And use ULONG_MAX to match
 strtoul.


 Index: lib/libssl/src/crypto/conf/conf_api.c
 ===
 RCS file: /cvs/src/lib/libssl/src/crypto/conf/conf_api.c,v
 retrieving revision 1.11
 diff -u -b -w -p -r1.11 conf_api.c
 --- lib/libssl/src/crypto/conf/conf_api.c23 Jun 2014 22:19:02
 -1.11
 +++ lib/libssl/src/crypto/conf/conf_api.c13 Jul 2014 07:43:09 -
 @@ -295,7 +295,7 @@ _CONF_new_section(CONF *conf, const char
 if ((v-section = malloc(i)) == NULL)
 goto err;

 -memcpy(v-section, section, i);
 +memmove(v-section, section, i);
 v-name = NULL;
 v-value = (char *)sk;
 Unrelated, but also unnecessary. The malloc above makes it clear
 v-section is a unique pointer not aliased with section. memcpy is fine.


 



signature.asc
Description: OpenPGP digital signature

[PATCH] fix overflow handling in dd(1)

[William Orr]

Hey,

I sent a patch similar to this almost a month ago with no response.

Feedback? Interest?

This patch fixes the following:

- Takes negative values
- When SIZE_T_MAX was passed, returns undefined error

Index: bin/dd/args.c
===
RCS file: /cvs/src/bin/dd/args.c,v
retrieving revision 1.25
diff -u -b -w -p -r1.25 args.c
--- bin/dd/args.c   21 May 2014 06:23:02 -  1.25
+++ bin/dd/args.c   13 Jul 2014 07:43:07 -
@@ -37,6 +37,7 @@
 #include sys/types.h
 #include sys/time.h
 
+#include ctype.h
 #include err.h
 #include errno.h
 #include limits.h
@@ -196,8 +197,7 @@ static void
 f_count(char *arg)
 {
 
-   if ((cpy_cnt = get_bsz(arg)) == 0)
-   cpy_cnt = (size_t)-1;
+   cpy_cnt = get_bsz(arg);
 }
 
 static void
@@ -322,9 +322,16 @@ get_bsz(char *val)
 {
size_t num, t;
char *expr;
+   char *vp = val;
 
-   num = strtoul(val, expr, 0);
-   if (num == SIZE_T_MAX)  /* Overflow. */
+   while (isspace(vp[0]))
+   vp++;
+   if (vp[0] == '-')
+   errx(1, %s: cannot be negative, oper);
+
+   errno = 0;
+   num = strtoul(vp, expr, 0);
+   if (num == SIZE_T_MAX  errno == ERANGE)   /* Overflow. */
err(1, %s, oper);
if (expr == val)/* No digits. */
errx(1, %s: illegal numeric value, oper);
Index: bin/dd/dd.c
===
RCS file: /cvs/src/bin/dd/dd.c,v
retrieving revision 1.18
diff -u -b -w -p -r1.18 dd.c
--- bin/dd/dd.c 1 Jun 2013 16:46:49 -   1.18
+++ bin/dd/dd.c 13 Jul 2014 07:43:07 -
@@ -77,7 +77,7 @@ main(int argc, char *argv[])
 
atexit(summary);
 
-   if (cpy_cnt != (size_t)-1) {
+   if (cpy_cnt != 0) {
while (files_cnt--)
dd_in();
}
Index: lib/libssl/src/crypto/conf/conf_api.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/conf/conf_api.c,v
retrieving revision 1.11
diff -u -b -w -p -r1.11 conf_api.c
--- lib/libssl/src/crypto/conf/conf_api.c   23 Jun 2014 22:19:02 -  
1.11
+++ lib/libssl/src/crypto/conf/conf_api.c   13 Jul 2014 07:43:09 -
@@ -295,7 +295,7 @@ _CONF_new_section(CONF *conf, const char
if ((v-section = malloc(i)) == NULL)
goto err;
 
-   memcpy(v-section, section, i);
+   memmove(v-section, section, i);
v-name = NULL;
v-value = (char *)sk;
 

Re: [PATCH] fix overflow handling in dd(1)

[Ted Unangst]

On Sun, Jul 13, 2014 at 01:52, William Orr wrote:
 Hey,
 
 I sent a patch similar to this almost a month ago with no response.
 
 Feedback? Interest?

Yes.

 
 - num = strtoul(val, expr, 0);
 - if (num == SIZE_T_MAX)  /* Overflow. */
 + while (isspace(vp[0]))
 + vp++;
 + if (vp[0] == '-')
 + errx(1, %s: cannot be negative, oper);
 +
 + errno = 0;
 + num = strtoul(vp, expr, 0);
 + if (num == SIZE_T_MAX  errno == ERANGE)   /* Overflow. */

I think you can just use strchr to look for a - anywhere in the
string. It shouldn't be anywhere, right? And use ULONG_MAX to match
strtoul.


 Index: lib/libssl/src/crypto/conf/conf_api.c
 ===
 RCS file: /cvs/src/lib/libssl/src/crypto/conf/conf_api.c,v
 retrieving revision 1.11
 diff -u -b -w -p -r1.11 conf_api.c
 --- lib/libssl/src/crypto/conf/conf_api.c 23 Jun 2014 22:19:02 -  
 1.11
 +++ lib/libssl/src/crypto/conf/conf_api.c 13 Jul 2014 07:43:09 -
 @@ -295,7 +295,7 @@ _CONF_new_section(CONF *conf, const char
 if ((v-section = malloc(i)) == NULL)
 goto err;
 
 - memcpy(v-section, section, i);
 + memmove(v-section, section, i);
 v-name = NULL;
 v-value = (char *)sk;

Unrelated, but also unnecessary. The malloc above makes it clear
v-section is a unique pointer not aliased with section. memcpy is fine.

Re: [PATCH] fix overflow handling in dd(1)

[William Orr]

Sorry, the libssl patch was unintentional. I forgot to cvs up -C that one.

On 7/13/2014 2:05 AM, Ted Unangst wrote:

On Sun, Jul 13, 2014 at 01:52, William Orr wrote:

Hey,

I sent a patch similar to this almost a month ago with no response.

Feedback? Interest?

Yes.


-   num = strtoul(val, expr, 0);
-   if (num == SIZE_T_MAX)  /* Overflow. */
+   while (isspace(vp[0]))
+   vp++;
+   if (vp[0] == '-')
+   errx(1, %s: cannot be negative, oper);
+
+   errno = 0;
+   num = strtoul(vp, expr, 0);
+   if (num == SIZE_T_MAX  errno == ERANGE)   /* Overflow. */

I think you can just use strchr to look for a - anywhere in the
string. It shouldn't be anywhere, right? And use ULONG_MAX to match
strtoul.



Index: lib/libssl/src/crypto/conf/conf_api.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/conf/conf_api.c,v
retrieving revision 1.11
diff -u -b -w -p -r1.11 conf_api.c
--- lib/libssl/src/crypto/conf/conf_api.c   23 Jun 2014 22:19:02 -  
1.11
+++ lib/libssl/src/crypto/conf/conf_api.c   13 Jul 2014 07:43:09 -
@@ -295,7 +295,7 @@ _CONF_new_section(CONF *conf, const char
if ((v-section = malloc(i)) == NULL)
goto err;

-   memcpy(v-section, section, i);
+   memmove(v-section, section, i);
v-name = NULL;
v-value = (char *)sk;

Unrelated, but also unnecessary. The malloc above makes it clear
v-section is a unique pointer not aliased with section. memcpy is fine.

Re: [PATCH] fix overflow handling in dd(1)

[William Orr]

Here is the latest diff with the bullshit removed and the loop replaced with 
strchr.

Index: bin/dd/args.c
===
RCS file: /cvs/src/bin/dd/args.c,v
retrieving revision 1.25
diff -u -b -w -p -r1.25 args.c
--- bin/dd/args.c   21 May 2014 06:23:02 -  1.25
+++ bin/dd/args.c   13 Jul 2014 09:13:18 -
@@ -196,8 +196,7 @@ static void
 f_count(char *arg)
 {
 
-	if ((cpy_cnt = get_bsz(arg)) == 0)

-   cpy_cnt = (size_t)-1;
+   cpy_cnt = get_bsz(arg);
 }
 
 static void

@@ -323,8 +322,12 @@ get_bsz(char *val)
size_t num, t;
char *expr;
 
-	num = strtoul(val, expr, 0);

-   if (num == SIZE_T_MAX)  /* Overflow. */
+   if (strchr(val, '-'))
+   errx(1, %s: illegal numeric value, oper);
+
+   errno = 0;
+   num = strtoul(val, expr, 0);
+   if (num == ULONG_MAX  errno == ERANGE)/* Overflow. */
err(1, %s, oper);
if (expr == val)/* No digits. */
errx(1, %s: illegal numeric value, oper);
Index: bin/dd/dd.c
===
RCS file: /cvs/src/bin/dd/dd.c,v
retrieving revision 1.18
diff -u -b -w -p -r1.18 dd.c
--- bin/dd/dd.c 1 Jun 2013 16:46:49 -   1.18
+++ bin/dd/dd.c 13 Jul 2014 09:13:18 -
@@ -77,7 +77,7 @@ main(int argc, char *argv[])
 
 	atexit(summary);
 
-	if (cpy_cnt != (size_t)-1) {

+   if (cpy_cnt != 0) {
while (files_cnt--)
dd_in();
}

On 7/13/2014 2:08 AM, William Orr wrote:
Sorry, the libssl patch was unintentional. I forgot to cvs up -C that 
one.


On 7/13/2014 2:05 AM, Ted Unangst wrote:

On Sun, Jul 13, 2014 at 01:52, William Orr wrote:

Hey,

I sent a patch similar to this almost a month ago with no response.

Feedback? Interest?

Yes.


-num = strtoul(val, expr, 0);
-if (num == SIZE_T_MAX)/* Overflow. */
+while (isspace(vp[0]))
+vp++;
+if (vp[0] == '-')
+errx(1, %s: cannot be negative, oper);
+
+errno = 0;
+num = strtoul(vp, expr, 0);
+if (num == SIZE_T_MAX  errno == ERANGE) /* Overflow. */

I think you can just use strchr to look for a - anywhere in the
string. It shouldn't be anywhere, right? And use ULONG_MAX to match
strtoul.



Index: lib/libssl/src/crypto/conf/conf_api.c
===
RCS file: /cvs/src/lib/libssl/src/crypto/conf/conf_api.c,v
retrieving revision 1.11
diff -u -b -w -p -r1.11 conf_api.c
--- lib/libssl/src/crypto/conf/conf_api.c23 Jun 2014 22:19:02 
-1.11

+++ lib/libssl/src/crypto/conf/conf_api.c13 Jul 2014 07:43:09 -
@@ -295,7 +295,7 @@ _CONF_new_section(CONF *conf, const char
if ((v-section = malloc(i)) == NULL)
goto err;

-memcpy(v-section, section, i);
+memmove(v-section, section, i);
v-name = NULL;
v-value = (char *)sk;

Unrelated, but also unnecessary. The malloc above makes it clear
v-section is a unique pointer not aliased with section. memcpy is fine.