[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: undiagnosed integer overflow in parsing frozen files
|
From: |
Eric Blake |
|
Subject: |
Re: undiagnosed integer overflow in parsing frozen files |
|
Date: |
Fri, 9 May 2008 17:09:07 +0000 (UTC) |
|
User-agent: |
Loom/3.14 (http://gmane.org/) |
Eric Blake <ebb9 <at> byu.net> writes:
> According to Jim Meyering on 5/8/2008 1:10 PM:
> |
> | However, given too long a string of digits, "Number" overflows.
>
> Accidental, but not unnoticed, and hopefully not severe.
> But now that you mention it, I'll
> probably tighten up the check
Like this.
>From b5fe50a4ef3e20912ee835888c9effda8a290721 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 9 May 2008 07:59:51 -0600
Subject: [PATCH] Detect integer overflow when loading frozen file.
* src/freeze.c (reload_frozen_state) [GET_NUMBER]: Rewrite to fail
immediately on overflow.
Reported by Jim Meyering.
Signed-off-by: Eric Blake <address@hidden>
---
ChangeLog | 5 +++++
src/freeze.c | 31 ++++++++++++++++++-------------
2 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 0a40526..fc894b1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
2008-05-09 Eric Blake <address@hidden>
+ Detect integer overflow when loading frozen file.
+ * src/freeze.c (reload_frozen_state) [GET_NUMBER]: Rewrite to fail
+ immediately on overflow.
+ Reported by Jim Meyering.
+
Stage 23: allow tracing of indirect macro calls.
Track all trace information as part of the argv struct, rather
than temporarily resetting global state. Teach indir to trace
diff --git a/src/freeze.c b/src/freeze.c
index 383d008..16ed75e 100644
--- a/src/freeze.c
+++ b/src/freeze.c
@@ -179,33 +179,38 @@ reload_frozen_state (const char *name)
int number[2];
const builtin *bp;
-#define GET_CHARACTER \
+#define GET_CHARACTER \
(character = getc (file))
-#define GET_NUMBER(Number) \
+#define GET_NUMBER(Number, AllowNeg) \
do \
{ \
- (Number) = 0; \
- while (isdigit (character)) \
+ unsigned int n = 0; \
+ while (isdigit (character) && n <= INT_MAX / 10) \
{ \
- (Number) = 10 * (Number) + character - '0'; \
+ n = 10 * n + character - '0'; \
GET_CHARACTER; \
} \
+ if (((AllowNeg) ? INT_MIN : INT_MAX) < n \
+ || isdigit (character)) \
+ m4_error (EXIT_FAILURE, 0, NULL, \
+ _("integer overflow in frozen file")); \
+ (Number) = n; \
} \
while (0)
-#define VALIDATE(Expected) \
+#define VALIDATE(Expected) \
do \
{ \
if (character != (Expected)) \
- issue_expect_message ((Expected)); \
+ issue_expect_message (Expected); \
} \
while (0)
/* Skip comments (`#' at beginning of line) and blank lines, setting
character to the next directive or to EOF. */
-#define GET_DIRECTIVE \
+#define GET_DIRECTIVE \
do \
{ \
GET_CHARACTER; \
@@ -215,7 +220,7 @@ reload_frozen_state (const char *name)
GET_CHARACTER; \
VALIDATE ('\n'); \
} \
- } \
+ } \
while (character == '\n')
file = m4_path_search (name, NULL);
@@ -231,7 +236,7 @@ reload_frozen_state (const char *name)
GET_DIRECTIVE;
VALIDATE ('V');
GET_CHARACTER;
- GET_NUMBER (number[0]);
+ GET_NUMBER (number[0], false);
if (number[0] > 1)
m4_error (EXIT_MISMATCH, 0, NULL,
_("frozen file version %d greater than max supported of 1"),
@@ -262,14 +267,14 @@ reload_frozen_state (const char *name)
if (operation == 'D' && character == '-')
{
GET_CHARACTER;
- GET_NUMBER (number[0]);
+ GET_NUMBER (number[0], true);
number[0] = -number[0];
}
else
- GET_NUMBER (number[0]);
+ GET_NUMBER (number[0], false);
VALIDATE (',');
GET_CHARACTER;
- GET_NUMBER (number[1]);
+ GET_NUMBER (number[1], false);
VALIDATE ('\n');
if (operation != 'D')
--
1.5.5.1
>From 34985ceaf524a511894776fe446279b5a7c12ced Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 9 May 2008 09:24:41 -0600
Subject: [PATCH] Improve error message when frozen file is invalid.
* src/freeze.c (reload_frozen_state): Track current line.
[GET_STRING]: New helper macro.
Signed-off-by: Eric Blake <address@hidden>
---
ChangeLog | 4 +++
src/freeze.c | 79 ++++++++++++++++++++++++++++++++-------------------------
2 files changed, 48 insertions(+), 35 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index fc894b1..2abc489 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
2008-05-09 Eric Blake <address@hidden>
+ Improve error message when frozen file is invalid.
+ * src/freeze.c (reload_frozen_state): Track current line.
+ [GET_STRING]: New helper macro.
+
Detect integer overflow when loading frozen file.
* src/freeze.c (reload_frozen_state) [GET_NUMBER]: Rewrite to fail
immediately on overflow.
diff --git a/src/freeze.c b/src/freeze.c
index 16ed75e..15f06fe 100644
--- a/src/freeze.c
+++ b/src/freeze.c
@@ -178,9 +178,21 @@ reload_frozen_state (const char *name)
int allocated[2];
int number[2];
const builtin *bp;
+ bool advance_line = true;
#define GET_CHARACTER \
- (character = getc (file))
+ do \
+ { \
+ if (advance_line) \
+ { \
+ current_line++; \
+ advance_line = false; \
+ } \
+ (character = getc (file)); \
+ if (character == '\n') \
+ advance_line = true; \
+ } \
+ while (0)
#define GET_NUMBER(Number, AllowNeg) \
do \
@@ -223,9 +235,35 @@ reload_frozen_state (const char *name)
} \
while (character == '\n')
+#define GET_STRING(i) \
+ do \
+ { \
+ void *tmp; \
+ char *p; \
+ if (number[(i)] + 1 > allocated[(i)]) \
+ { \
+ free (string[(i)]); \
+ allocated[(i)] = number[(i)] + 1; \
+ string[(i)] = xcharalloc ((size_t) allocated[(i)]); \
+ } \
+ if (number[(i)] > 0 \
+ && !fread (string[(i)], (size_t) number[(i)], 1, file)) \
+ m4_error (EXIT_FAILURE, 0, NULL, \
+ _("premature end of frozen file")); \
+ string[(i)][number[(i)]] = '\0'; \
+ p = string[(i)]; \
+ while ((tmp = memchr(p, '\n', number[(i)] - (p - string[(i)])))) \
+ { \
+ current_line++; \
+ p = (char *) tmp + 1; \
+ } \
+ } \
+ while (0)
+
file = m4_path_search (name, NULL);
if (file == NULL)
m4_error (EXIT_FAILURE, errno, NULL, _("cannot open %s"), name);
+ current_file = name;
allocated[0] = 100;
string[0] = xcharalloc ((size_t) allocated[0]);
@@ -278,40 +316,8 @@ reload_frozen_state (const char *name)
VALIDATE ('\n');
if (operation != 'D')
- {
-
- /* Get first string contents. */
-
- if (number[0] + 1 > allocated[0])
- {
- free (string[0]);
- allocated[0] = number[0] + 1;
- string[0] = xcharalloc ((size_t) allocated[0]);
- }
-
- if (number[0] > 0)
- if (!fread (string[0], (size_t) number[0], 1, file))
- m4_error (EXIT_FAILURE, 0, NULL,
- _("premature end of frozen file"));
-
- string[0][number[0]] = '\0';
- }
-
- /* Get second string contents. */
-
- if (number[1] + 1 > allocated[1])
- {
- free (string[1]);
- allocated[1] = number[1] + 1;
- string[1] = xcharalloc ((size_t) allocated[1]);
- }
-
- if (number[1] > 0)
- if (!fread (string[1], (size_t) number[1], 1, file))
- m4_error (EXIT_FAILURE, 0, NULL,
- _("premature end of frozen file"));
-
- string[1][number[1]] = '\0';
+ GET_STRING (0);
+ GET_STRING (1);
GET_CHARACTER;
VALIDATE ('\n');
@@ -375,9 +381,12 @@ reload_frozen_state (const char *name)
errno = 0;
if (ferror (file) || fclose (file) != 0)
m4_error (EXIT_FAILURE, errno, NULL, _("unable to read frozen state"));
+ current_file = NULL;
+ current_line = 0;
#undef GET_CHARACTER
#undef GET_DIRECTIVE
#undef GET_NUMBER
#undef VALIDATE
+#undef GET_STRING
}
--
1.5.5.1