[PATCH 10/10] libiberty: Correct an invalid assumption

[PATCH 10/10] libiberty: Correct an invalid assumption

[Ben L]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

Hi all,

First time emailing gcc-patches, so I'm sorry if I get any of this wrong or if
there's obvious errors repeated in my patches. AFAICT I should be sending each
change individually rather than as one bulk patch, so I'm sorry about the spam
too.

All of these changes were found by fuzzing libiberty's demanglers over the
past week, and I have at least one more that it's currently crashing out on
but I haven't had time to look into why yet.

Obviously since this is my first time emailing I don't have write access to
commit any of these, so if any are approved then I'd be grateful if you can
commit them too.

Thanks,
Ben

--

As a counter example: 8888888888888888888 * 10 = -3344831479658869200, which is
valid for 64 bit longs, and evidently divisible by 10.

Also safely check that adding the digit won't cause an overflow too.

No testcase provided since one of the previous testcases flagged this issue up.

     * d-demangle.c: Include <limits.h> if available.
     (LONG_MAX): Define if necessary.
     (dlang_number): Fix overflow.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0010-libiberty-Correct-an-invalid-assumption.patch --]
[-- Type: text/x-patch; name="0010-libiberty-Correct-an-invalid-assumption.patch", Size: 1687 bytes --]

From 6dc14e124c4a48928046403faca37504229b13c4 Mon Sep 17 00:00:00 2001
From: bobsayshilol <bobsayshilol@live.co.uk>
Date: Wed, 9 Jan 2019 22:57:08 +0000
Subject: [PATCH 10/10] libiberty: Correct an invalid assumption.

As a counter example: 8888888888888888888 * 10 = -3344831479658869200, which is
valid for 64 bit longs, and evidently divisible by 10.

Also safely check that adding the digit won't cause an overflow too.

No testcase provided since one of the previous testcases flagged this issue up.

    * d-demangle.c: Include <limits.h> if available.
    (LONG_MAX): Define if necessary.
    (dlang_number): Fix overflow.

diff --git a/libiberty/d-demangle.c b/libiberty/d-demangle.c
index becc402..4ffcdd1 100644
--- a/libiberty/d-demangle.c
+++ b/libiberty/d-demangle.c
@@ -42,6 +42,13 @@ If not, see <http://www.gnu.org/licenses/>.  */
 #include <stdlib.h>
 #endif
 
+#ifdef HAVE_LIMITS_H
+#include <limits.h>
+#endif
+#ifndef LONG_MAX
+# define LONG_MAX      (long)(((unsigned long) ~0) >> 1)
+#endif
+
 #include <demangle.h>
 #include "libiberty.h"
 
@@ -206,15 +213,18 @@ dlang_number (const char *mangled, long *ret)
 
   while (ISDIGIT (*mangled))
     {
+      long digit = mangled[0] - '0';
+      mangled++;
+
+      if (*ret > LONG_MAX / 10)
+	return NULL;
+
       (*ret) *= 10;
 
-      /* If an overflow occured when multiplying by ten, the result
-	 will not be a multiple of ten.  */
-      if ((*ret % 10) != 0)
+      if (LONG_MAX - digit < *ret)
 	return NULL;
 
-      (*ret) += mangled[0] - '0';
-      mangled++;
+      (*ret) += digit;
     }
 
   if (*mangled == '\0' || *ret < 0)
-- 
2.20.1

Re: [PATCH 10/10] libiberty: Correct an invalid assumption

[Iain Buclaw]

On Fri, 11 Jan 2019 at 01:20, Ben L <bobsayshilol@live.co.uk> wrote:
>
> Hi all,
>
> First time emailing gcc-patches, so I'm sorry if I get any of this wrong or if
> there's obvious errors repeated in my patches. AFAICT I should be sending each
> change individually rather than as one bulk patch, so I'm sorry about the spam
> too.
>
> All of these changes were found by fuzzing libiberty's demanglers over the
> past week, and I have at least one more that it's currently crashing out on
> but I haven't had time to look into why yet.
>
> Obviously since this is my first time emailing I don't have write access to
> commit any of these, so if any are approved then I'd be grateful if you can
> commit them too.
>
> Thanks,
> Ben
>
> --
>
> As a counter example: 8888888888888888888 * 10 = -3344831479658869200, which is
> valid for 64 bit longs, and evidently divisible by 10.
>
> Also safely check that adding the digit won't cause an overflow too.
>
> No testcase provided since one of the previous testcases flagged this issue up.
>
>      * d-demangle.c: Include <limits.h> if available.
>      (LONG_MAX): Define if necessary.
>      (dlang_number): Fix overflow.
>

Thanks, do you have a copyright assignment with the FSF?

Looks like the D demangling bits can just be committed as one patch,
just one nit though.

---
> @@ -206,15 +213,18 @@ dlang_number (const char *mangled, long *ret)
>
>   while (ISDIGIT (*mangled))
>     {
> +      long digit = mangled[0] - '0';
> +      mangled++;
> +
> +      if (*ret > LONG_MAX / 10)
> +       return NULL;
> +
>       (*ret) *= 10;
>
> -      /* If an overflow occured when multiplying by ten, the result
> -        will not be a multiple of ten.  */
> -      if ((*ret % 10) != 0)
> +      if (LONG_MAX - digit < *ret)
>        return NULL;
>
> -      (*ret) += mangled[0] - '0';
> -      mangled++;
> +      (*ret) += digit;
>      }
>
>    if (*mangled == '\0' || *ret < 0)
---

Rather than checking for overflow twice, I think it would be
sufficient to just do:
---
long digit = mangled[0] - '0';

if (*ret > ((LONG_MAX - digit) / 10))
  return NULL;

(*ret) *= 10;
(*ret) += digit;
mangled++;
---

-- 
Iain

Re: [PATCH 10/10] libiberty: Correct an invalid assumption

[Ben L]

On 14/01/2019 11:10, Iain Buclaw wrote:

> Thanks, do you have a copyright assignment with the FSF?

No problem, and no I don't think so. I'd assumed these patches were trivial
enough to not need anything like that, but if so then what do I need to do?

> Rather than checking for overflow twice, I think it would be
> sufficient to just do:
> ---
> long digit = mangled[0] - '0';
>
> if (*ret > ((LONG_MAX - digit) / 10))
>    return NULL;
>
> (*ret) *= 10;
> (*ret) += digit;
> mangled++;
> ---

I'd agree, that does the trick too. Do I need to resend the patch with that
change, or can that be done by whoever applies it since they'll be squashed
into a single patch anyway?

Thanks,
Ben