[PATCH] Coverity: regcomp.c etc. #13761
Comments
From @jhiNeeded compile-time limits for the PL_reg_intflags_name so that the Attached. |
From @jhi0009-Fix-for-Coverity-perl5-CID-29034.patchFrom be565fd45f2ca9507a266617cfeeaaed82d36c3f Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Mon, 21 Apr 2014 21:43:12 -0400
Subject: [PATCH 9/9] Fix for Coverity perl5 CID 29034: Out-of-bounds read
(OVERRUN) overrun-local: Overrunning array PL_reg_intflags name of 14 8-byte
elements at element index 31 (byte offset 248) using index bit (which
evaluates to 31).
Needed compile-time limits for the PL_reg_intflags_name so that the
bit loop doesn't waltz off past the array. Could not use C_ARRAY_LENGTH
because the size of name array is not visible during compile time
(only const char*[] is), so modified regcomp.pl to generate the size,
made it visible only for the re extension. Did extflags analogously
even though its size currently exactly 32 already. The sizeof(flags)*8
is extra paranoia for ILP64.
---
regcomp.c | 8 ++++++--
regen/regcomp.pl | 17 ++++++++++++++++-
regnodes.h | 8 ++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/regcomp.c b/regcomp.c
index 0238af9..66f5c84 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15365,7 +15365,9 @@ S_regdump_intflags(pTHX_ const char *lead, const U32 flags)
int bit;
int set=0;
- for (bit=0; bit<32; bit++) {
+ for (bit=0;
+ bit<REG_INTFLAGS_NAME_SIZE && bit<sizeof(flags)*8;
+ bit++) {
if (flags & (1<<bit)) {
if (!set++ && lead)
PerlIO_printf(Perl_debug_log, "%s",lead);
@@ -15387,7 +15389,9 @@ S_regdump_extflags(pTHX_ const char *lead, const U32 flags)
int set=0;
regex_charset cs;
- for (bit=0; bit<32; bit++) {
+ for (bit=0;
+ bit<REG_EXTFLAGS_NAME_SIZE && bit<sizeof(flags)*8;
+ bit++) {
if (flags & (1<<bit)) {
if ((1<<bit) & RXf_PMf_CHARSET) { /* Output separately, below */
continue;
diff --git a/regen/regcomp.pl b/regen/regcomp.pl
index 4a8b9d5..657ca72 100644
--- a/regen/regcomp.pl
+++ b/regen/regcomp.pl
@@ -261,6 +261,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_EXTFLAGS_NAME_SIZE = 0;
foreach my $file ("op_reg_common.h", "regexp.h") {
open FH,"<$file" or die "Can't read $file: $!";
while (<FH>) {
@@ -332,6 +333,7 @@ for (0..31) {
s/\bRXf_(PMf_)?// for $n, $extra;
printf $out qq(\t%-20s/* 0x%08x%s */\n),
qq("$n",),$power_of_2, $extra;
+ $REG_EXTFLAGS_NAME_SIZE++;
}
print $out <<EOP;
@@ -339,6 +341,12 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
+print $out <<EOQ
+#ifdef PERL_IN_XSUB_RE
+# define REG_EXTFLAGS_NAME_SIZE $REG_EXTFLAGS_NAME_SIZE /* sizeof(PL_reg_intflags_name) */
+#endif
+
+EOQ
}
{
print $out <<EOP;
@@ -354,6 +362,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_INTFLAGS_NAME_SIZE = 0;
foreach my $file ("regcomp.h") {
open my $fh, "<", $file or die "Can't read $file: $!";
while (<$fh>) {
@@ -369,6 +378,7 @@ foreach my $file ("regcomp.h") {
$comment= $comment ? " - $comment" : "";
printf $out qq(\t%-30s/* 0x%08x - %s%s */\n), qq("$abbr",), $val, $define, $comment;
+ $REG_INTFLAGS_NAME_SIZE++;
}
}
}
@@ -378,8 +388,13 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
-}
+print $out <<EOQ;
+#ifdef PERL_IN_XSUB_RE
+# define REG_INTFLAGS_NAME_SIZE $REG_INTFLAGS_NAME_SIZE /* sizeof(PL_reg_intflags_name) */
+#endif
+EOQ
+}
print $out process_flags('V', 'varies', <<'EOC');
/* The following have no fixed length. U8 so we can do strchr() on it. */
diff --git a/regnodes.h b/regnodes.h
index 4f4ff9e..9dae3af 100644
--- a/regnodes.h
+++ b/regnodes.h
@@ -676,6 +676,10 @@ EXTCONST char * const PL_reg_extflags_name[] = {
};
#endif /* DOINIT */
+#ifdef PERL_IN_XSUB_RE
+# define REG_EXTFLAGS_NAME_SIZE 32 /* sizeof(PL_reg_intflags_name) */
+#endif
+
/* PL_reg_intflags_name[] - Opcode/state names in string form, for debugging */
#ifndef DOINIT
@@ -699,6 +703,10 @@ EXTCONST char * const PL_reg_intflags_name[] = {
};
#endif /* DOINIT */
+#ifdef PERL_IN_XSUB_RE
+# define REG_INTFLAGS_NAME_SIZE 14 /* sizeof(PL_reg_intflags_name) */
+#endif
+
/* The following have no fixed length. U8 so we can do strchr() on it. */
#define REGNODE_VARIES(node) (PL_varies_bitmask[(node) >> 3] & (1 << ((node) & 7)))
--
1.8.5.2 (Apple Git-48)
|
From @jhiOn Tuesday-201404-22, 20:33, perlbug-followup@perl.org wrote:
Ugh, thought I pasted more to the Subject line... should've been: [PATCH] Coverity: regcomp.c etc. array access past the end
|
From @tonycozOn Tue Apr 22 17:33:07 2014, jhi wrote:
Wouldn't the paranoia in: + for (bit=0; be better handled with an assert? Even a compile-time assert if we had the mechanism for it. Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @jhiI guess one could assert that the number of names is less than or equal to
|
From @bulk88On Tue Apr 22 18:43:07 2014, tonyc wrote:
Add one. For example #define STATIC_ASSERT(expr) ((void)sizeof(char[1 - 2*!!!(expr)])) from https://metacpan.org/source/BULKDD/Win32-API-0.77/API.h#L248 I've added new C compiler support macros before in ae103e0#diff-408899f5d0d5a90cdff082c8f0d0441aR3111 . -- |
From @hvdsThere seems also to be a copy/paste in the comments: I assume both should s/int/ext/. Hugo |
From @jhiOn Wednesday-201404-23, 15:06, Hugo van der Sanden via RT wrote:
Ah, yes. I'll reissue the patch. Where is my C compiler which proofreds
|
From @jhiOn Wednesday-201404-23, 4:38, bulk88 via RT wrote:
Sir, please back away from the keyboard and keep your hands above your |
From @jhiOn Wednesday-201404-23, 15:06, Hugo van der Sanden via RT wrote:
The updated patch attached. It also includes the suggestion to use assert(), though I went ahead and (Further change from the earlier incarnation is that the new symbols |
From @jhi0001-Fix-for-Coverity-perl5-CID-29034.patchFrom e88c498f5375232e4e18339221c757af026f7b92 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Mon, 21 Apr 2014 21:43:12 -0400
Subject: [PATCH] Fix for Coverity perl5 CID 29034: Out-of-bounds read
(OVERRUN) overrun-local: Overrunning array PL_reg_intflags name of 14 8-byte
elements at element index 31 (byte offset 248) using index bit (which
evaluates to 31).
Needed compile-time limits for the PL_reg_intflags_name so that the
bit loop doesn't waltz off past the array. Could not use C_ARRAY_LENGTH
because the size of name array is not visible during compile time
(only const char*[] is), so modified regcomp.pl to generate the size,
made it visible only under DEBUGGING. Did extflags analogously
even though its size currently exactly 32 already. The sizeof(flags)*8
is extra paranoia for ILP64.
---
regcomp.c | 8 ++++++--
regen/regcomp.pl | 17 ++++++++++++++++-
regnodes.h | 8 ++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/regcomp.c b/regcomp.c
index 0238af9..920f7cb 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15365,7 +15365,9 @@ S_regdump_intflags(pTHX_ const char *lead, const U32 flags)
int bit;
int set=0;
- for (bit=0; bit<32; bit++) {
+ ASSUME(REG_INTFLAGS_NAME_SIZE <= sizeof(flags)*8);
+
+ for (bit=0; bit<REG_INTFLAGS_NAME_SIZE; bit++) {
if (flags & (1<<bit)) {
if (!set++ && lead)
PerlIO_printf(Perl_debug_log, "%s",lead);
@@ -15387,7 +15389,9 @@ S_regdump_extflags(pTHX_ const char *lead, const U32 flags)
int set=0;
regex_charset cs;
- for (bit=0; bit<32; bit++) {
+ ASSUME(REG_EXTFLAGS_NAME_SIZE <= sizeof(flags)*8);
+
+ for (bit=0; bit<REG_EXTFLAGS_NAME_SIZE; bit++) {
if (flags & (1<<bit)) {
if ((1<<bit) & RXf_PMf_CHARSET) { /* Output separately, below */
continue;
diff --git a/regen/regcomp.pl b/regen/regcomp.pl
index 4a8b9d5..779545d 100644
--- a/regen/regcomp.pl
+++ b/regen/regcomp.pl
@@ -261,6 +261,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_EXTFLAGS_NAME_SIZE = 0;
foreach my $file ("op_reg_common.h", "regexp.h") {
open FH,"<$file" or die "Can't read $file: $!";
while (<FH>) {
@@ -332,6 +333,7 @@ for (0..31) {
s/\bRXf_(PMf_)?// for $n, $extra;
printf $out qq(\t%-20s/* 0x%08x%s */\n),
qq("$n",),$power_of_2, $extra;
+ $REG_EXTFLAGS_NAME_SIZE++;
}
print $out <<EOP;
@@ -339,6 +341,12 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
+print $out <<EOQ
+#ifdef DEBUGGING
+# define REG_EXTFLAGS_NAME_SIZE $REG_EXTFLAGS_NAME_SIZE /* sizeof(PL_reg_extflags_name) */
+#endif
+
+EOQ
}
{
print $out <<EOP;
@@ -354,6 +362,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_INTFLAGS_NAME_SIZE = 0;
foreach my $file ("regcomp.h") {
open my $fh, "<", $file or die "Can't read $file: $!";
while (<$fh>) {
@@ -369,6 +378,7 @@ foreach my $file ("regcomp.h") {
$comment= $comment ? " - $comment" : "";
printf $out qq(\t%-30s/* 0x%08x - %s%s */\n), qq("$abbr",), $val, $define, $comment;
+ $REG_INTFLAGS_NAME_SIZE++;
}
}
}
@@ -378,8 +388,13 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
-}
+print $out <<EOQ;
+#ifdef DEBUGGING
+# define REG_INTFLAGS_NAME_SIZE $REG_INTFLAGS_NAME_SIZE /* sizeof(PL_reg_intflags_name) */
+#endif
+EOQ
+}
print $out process_flags('V', 'varies', <<'EOC');
/* The following have no fixed length. U8 so we can do strchr() on it. */
diff --git a/regnodes.h b/regnodes.h
index 4f4ff9e..71cc205 100644
--- a/regnodes.h
+++ b/regnodes.h
@@ -676,6 +676,10 @@ EXTCONST char * const PL_reg_extflags_name[] = {
};
#endif /* DOINIT */
+#ifdef DEBUGGING
+# define REG_EXTFLAGS_NAME_SIZE 32 /* sizeof(PL_reg_extflags_name) */
+#endif
+
/* PL_reg_intflags_name[] - Opcode/state names in string form, for debugging */
#ifndef DOINIT
@@ -699,6 +703,10 @@ EXTCONST char * const PL_reg_intflags_name[] = {
};
#endif /* DOINIT */
+#ifdef DEBUGGING
+# define REG_INTFLAGS_NAME_SIZE 14 /* sizeof(PL_reg_intflags_name) */
+#endif
+
/* The following have no fixed length. U8 so we can do strchr() on it. */
#define REGNODE_VARIES(node) (PL_varies_bitmask[(node) >> 3] & (1 << ((node) & 7)))
--
1.9.2
|
From @tonycozOn Wed Apr 23 16:59:42 2014, jhi wrote:
+#ifdef DEBUGGING It's not really sizeof(), but the element count. Outside of that I think it's good. Tony |
From @jhiOn Monday-201404-28, 20:32, Tony Cook via RT wrote:
Do you want me to regen the patch (with the comment removed, I think)? |
From @jhiOn Monday-201404-28, 20:40, Jarkko Hietaniemi wrote:
Regened patch attached. |
From @jhi0001-Fix-for-Coverity-perl5-CID-29034.patchFrom f287c8d21af3dc56b48ec8d7268d3d92c6688434 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Mon, 21 Apr 2014 21:43:12 -0400
Subject: [PATCH] Fix for Coverity perl5 CID 29034: Out-of-bounds read
(OVERRUN) overrun-local: Overrunning array PL_reg_intflags name of 14 8-byte
elements at element index 31 (byte offset 248) using index bit (which
evaluates to 31).
Needed compile-time limits for the PL_reg_intflags_name so that the
bit loop doesn't waltz off past the array. Could not use C_ARRAY_LENGTH
because the size of name array is not visible during compile time
(only const char*[] is), so modified regcomp.pl to generate the size,
made it visible only under DEBUGGING. Did extflags analogously
even though its size currently exactly 32 already. The sizeof(flags)*8
is extra paranoia for ILP64.
---
regcomp.c | 8 ++++++--
regen/regcomp.pl | 17 ++++++++++++++++-
regnodes.h | 8 ++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/regcomp.c b/regcomp.c
index 0238af9..920f7cb 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15365,7 +15365,9 @@ S_regdump_intflags(pTHX_ const char *lead, const U32 flags)
int bit;
int set=0;
- for (bit=0; bit<32; bit++) {
+ ASSUME(REG_INTFLAGS_NAME_SIZE <= sizeof(flags)*8);
+
+ for (bit=0; bit<REG_INTFLAGS_NAME_SIZE; bit++) {
if (flags & (1<<bit)) {
if (!set++ && lead)
PerlIO_printf(Perl_debug_log, "%s",lead);
@@ -15387,7 +15389,9 @@ S_regdump_extflags(pTHX_ const char *lead, const U32 flags)
int set=0;
regex_charset cs;
- for (bit=0; bit<32; bit++) {
+ ASSUME(REG_EXTFLAGS_NAME_SIZE <= sizeof(flags)*8);
+
+ for (bit=0; bit<REG_EXTFLAGS_NAME_SIZE; bit++) {
if (flags & (1<<bit)) {
if ((1<<bit) & RXf_PMf_CHARSET) { /* Output separately, below */
continue;
diff --git a/regen/regcomp.pl b/regen/regcomp.pl
index 4a8b9d5..2b6d964 100644
--- a/regen/regcomp.pl
+++ b/regen/regcomp.pl
@@ -261,6 +261,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_EXTFLAGS_NAME_SIZE = 0;
foreach my $file ("op_reg_common.h", "regexp.h") {
open FH,"<$file" or die "Can't read $file: $!";
while (<FH>) {
@@ -332,6 +333,7 @@ for (0..31) {
s/\bRXf_(PMf_)?// for $n, $extra;
printf $out qq(\t%-20s/* 0x%08x%s */\n),
qq("$n",),$power_of_2, $extra;
+ $REG_EXTFLAGS_NAME_SIZE++;
}
print $out <<EOP;
@@ -339,6 +341,12 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
+print $out <<EOQ
+#ifdef DEBUGGING
+# define REG_EXTFLAGS_NAME_SIZE $REG_EXTFLAGS_NAME_SIZE
+#endif
+
+EOQ
}
{
print $out <<EOP;
@@ -354,6 +362,7 @@ my %rxfv;
my %definitions; # Remember what the symbol definitions are
my $val = 0;
my %reverse;
+my $REG_INTFLAGS_NAME_SIZE = 0;
foreach my $file ("regcomp.h") {
open my $fh, "<", $file or die "Can't read $file: $!";
while (<$fh>) {
@@ -369,6 +378,7 @@ foreach my $file ("regcomp.h") {
$comment= $comment ? " - $comment" : "";
printf $out qq(\t%-30s/* 0x%08x - %s%s */\n), qq("$abbr",), $val, $define, $comment;
+ $REG_INTFLAGS_NAME_SIZE++;
}
}
}
@@ -378,8 +388,13 @@ print $out <<EOP;
#endif /* DOINIT */
EOP
-}
+print $out <<EOQ;
+#ifdef DEBUGGING
+# define REG_INTFLAGS_NAME_SIZE $REG_INTFLAGS_NAME_SIZE
+#endif
+EOQ
+}
print $out process_flags('V', 'varies', <<'EOC');
/* The following have no fixed length. U8 so we can do strchr() on it. */
diff --git a/regnodes.h b/regnodes.h
index 4f4ff9e..43ec681 100644
--- a/regnodes.h
+++ b/regnodes.h
@@ -676,6 +676,10 @@ EXTCONST char * const PL_reg_extflags_name[] = {
};
#endif /* DOINIT */
+#ifdef DEBUGGING
+# define REG_EXTFLAGS_NAME_SIZE 32
+#endif
+
/* PL_reg_intflags_name[] - Opcode/state names in string form, for debugging */
#ifndef DOINIT
@@ -699,6 +703,10 @@ EXTCONST char * const PL_reg_intflags_name[] = {
};
#endif /* DOINIT */
+#ifdef DEBUGGING
+# define REG_INTFLAGS_NAME_SIZE 14
+#endif
+
/* The following have no fixed length. U8 so we can do strchr() on it. */
#define REGNODE_VARIES(node) (PL_varies_bitmask[(node) >> 3] & (1 << ((node) & 7)))
--
1.9.2
|
From @tonycozOn Tue Apr 29 05:09:17 2014, jhi wrote:
Thanks, added as a 5.20 blocker and applied as adc2d0c. Tony |
|
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#121712 (status was 'resolved')
Searchable as RT121712$
The text was updated successfully, but these errors were encountered: