| Message ID | CY4PR22MB0102BB39DCD16A9A7A2E1F6DE7850@CY4PR22MB0102.namprd22.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] libiberty: Fix an out of bounds read in d_expression_1() | expand |
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++; ---
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
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
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.