[PATCH] `git log --pretty=format:%s HEAD^1..HEAD` #11988

p5pRT · 2012-03-06T19:53:34Z

Migrated from rt.perl.org#111586 (status was 'resolved')

Searchable as RT111586$

p5pRT · 2012-03-06T19:53:35Z

From @rurban

This is a bug report for perl from rurban@cpanel.net,
generated with the help of perlbug 1.39 running under perl 5.15.7.

From 657a387f083d6c36982ccecb1aa8df45426401ac Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer

ext/SDBM_File/sdbm/sdbm.c | 10 +++++-----
ext/SDBM_File/sdbm/sdbm.h | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)

Inline Patch

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
  */
 	filelen = strlen(file);
 
-	if ((dirname = (char *) malloc(filelen + dirfext_len + 1
-				       + filelen + pagfext_len + 1)) == NULL)
+	if ((dirname = (char *) malloc(filelen + dirfext_len
+				       + filelen + pagfext_len)) == NULL)
 		return errno = ENOMEM, (DBM *) NULL;
 /*
  * build the file names
  */
 	memcpy(dirname, file, filelen);
-	memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
-	pagname = dirname + filelen + dirfext_len + 1;
+	memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+	pagname = dirname + filelen + dirfext_len;
 	memcpy(pagname, file, filelen);
-	memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+	memcpy(pagname + filelen, PAGFEXT, pagfext_len);
 
 	db = sdbm_prep(dirname, pagname, flags, mode);
 	free((char *) dirname);
diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
 #define SPLTMAX	10			/* maximum allowed splits */
 					/* for a single insertion */
 #ifdef VMS
-#define DIRFEXT	".sdbm_dir"
+#define DIRFEXT	".sdbm_dir\0"
 #else
-#define DIRFEXT	".dir"
+#define DIRFEXT	".dir\0"
 #endif
-#define PAGFEXT	".pag"
+#define PAGFEXT	".pag\0"
 
 typedef struct {
 	int dirf;		       /* directory file descriptor */
--

1.7.5.4

Flags:
category=library
severity=critical
module=SDBM_File

Site configuration information for perl 5.15.7:

Configured by rurban at Sun Jan 22 12:13:34 CST 2012.

Summary of my perl5 (revision 5 version 15 subversion 7) configuration:

Platform:
osname=linux, osvers=3.0.0-1-amd64, archname=x86_64-linux
uname='linux reini 3.0.0-1-amd64 #1 smp sun jul 24 02:24:44 utc 2011 x86_64 gnulinux '
config_args='-de -Dusedevel -Dinstallman1dir=none -Dinstallman3dir=none -Dinstallsiteman1dir=none -Dinstallsiteman3dir=none -Uuseithreads -Accflags='-msse4.2' -Accflags='-march=corei7' -Dcf_email='rurban@cpanel.net' -Dperladmin='rurban@cpanel.net' -Duseshrplib'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=define, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cc', ccflags ='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-O2',
cppflags='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.6.1', gccosandvers=''
intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /lib /usr/lib /usr/lib/x86_64-linux-gnu /lib64 /usr/lib64
libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
libc=, so=so, useshrplib=true, libperl=libperl.so
gnulibc_version='2.13'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/lib/perl5/5.15.7/x86_64-linux/CORE'
cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:

@INC for perl 5.15.7:
/usr/local/lib/perl5/site_perl/5.15.7/x86_64-linux
/usr/local/lib/perl5/site_perl/5.15.7
/usr/local/lib/perl5/5.15.7/x86_64-linux
/usr/local/lib/perl5/5.15.7
/usr/local/lib/perl5/site_perl
.

Environment for perl 5.15.7:
HOME=/home/rurban
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH=/home/rurban/Perl/src/build-5.15.8d-nt-asan
LOGDIR (unset)
PATH=/home/rurban/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
PERL5LIB=
PERL5OPT=
PERL5_CPANPLUS_IS_RUNNING=441
PERL5_CPAN_IS_RUNNING=441
PERL_BADLANG (unset)
SHELL=/bin/bash

p5pRT · 2012-03-06T20:04:21Z

From @rurban

The attached patch is better, without the temp. comment at the code
line in question.

On Tue, Mar 6, 2012 at 1:53 PM, rurban@cpanel.net
<perlbug-followup@perl.org> wrote:

# New Ticket Created by rurban@cpanel.net
# Please include the string: [perl #111586]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=111586 >

This is a bug report for perl from rurban@cpanel.net,
generated with the help of perlbug 1.39 running under perl 5.15.7.

From 657a387f083d6c36982ccecb1aa8df45426401ac Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer
---
ext/SDBM_File/sdbm/sdbm.c | 10 +++++-----
ext/SDBM_File/sdbm/sdbm.h | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
*/
filelen = strlen(file);

- if ((dirname = (char *) malloc(filelen + dirfext_len + 1
- + filelen + pagfext_len + 1)) == NULL)
+ if ((dirname = (char *) malloc(filelen + dirfext_len
+ + filelen + pagfext_len)) == NULL)
return errno = ENOMEM, (DBM *) NULL;
/*
* build the file names
*/
memcpy(dirname, file, filelen);
- memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
- pagname = dirname + filelen + dirfext_len + 1;
+ memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+ pagname = dirname + filelen + dirfext_len;
memcpy(pagname, file, filelen);
- memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+ memcpy(pagname + filelen, PAGFEXT, pagfext_len);

db = sdbm_prep(dirname, pagname, flags, mode);
free((char *) dirname);
diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
#define SPLTMAX 10 /* maximum allowed splits */
/* for a single insertion */
#ifdef VMS
-#define DIRFEXT ".sdbm_dir"
+#define DIRFEXT ".sdbm_dir\0"
#else
-#define DIRFEXT ".dir"
+#define DIRFEXT ".dir\0"
#endif
-#define PAGFEXT ".pag"
+#define PAGFEXT ".pag\0"

typedef struct {
int dirf; /* directory file descriptor */
--
1.7.5.4

---
Flags:
category=library
severity=critical
module=SDBM_File
---
Site configuration information for perl 5.15.7:

Configured by rurban at Sun Jan 22 12:13:34 CST 2012.

Summary of my perl5 (revision 5 version 15 subversion 7) configuration:

Platform:
osname=linux, osvers=3.0.0-1-amd64, archname=x86_64-linux
uname='linux reini 3.0.0-1-amd64 #1 smp sun jul 24 02:24:44 utc 2011 x86_64 gnulinux '
config_args='-de -Dusedevel -Dinstallman1dir=none -Dinstallman3dir=none -Dinstallsiteman1dir=none -Dinstallsiteman3dir=none -Uuseithreads -Accflags='-msse4.2' -Accflags='-march=corei7' -Dcf_email='rurban@cpanel.net' -Dperladmin='rurban@cpanel.net' -Duseshrplib'
hint=recommended, useposix=true, d_sigaction=define
useithreads=undef, usemultiplicity=undef
useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
use64bitint=define, use64bitall=define, uselongdouble=undef
usemymalloc=n, bincompat5005=undef
Compiler:
cc='cc', ccflags ='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
optimize='-O2',
cppflags='-msse4.2 -march=corei7 -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
ccversion='', gccversion='4.6.1', gccosandvers=''
intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
alignbytes=8, prototype=define
Linker and Libraries:
ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
libpth=/usr/local/lib /lib /usr/lib /usr/lib/x86_64-linux-gnu /lib64 /usr/lib64
libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
libc=, so=so, useshrplib=true, libperl=libperl.so
gnulibc_version='2.13'
Dynamic Linking:
dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/lib/perl5/5.15.7/x86_64-linux/CORE'
cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:

---
@INC for perl 5.15.7:
/usr/local/lib/perl5/site_perl/5.15.7/x86_64-linux
/usr/local/lib/perl5/site_perl/5.15.7
/usr/local/lib/perl5/5.15.7/x86_64-linux
/usr/local/lib/perl5/5.15.7
/usr/local/lib/perl5/site_perl
.

---
Environment for perl 5.15.7:
HOME=/home/rurban
LANG=en_US.UTF-8
LANGUAGE (unset)
LD_LIBRARY_PATH=/home/rurban/Perl/src/build-5.15.8d-nt-asan
LOGDIR (unset)
PATH=/home/rurban/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
PERL5LIB=
PERL5OPT=
PERL5_CPANPLUS_IS_RUNNING=441
PERL5_CPAN_IS_RUNNING=441
PERL_BADLANG (unset)
SHELL=/bin/bash

--
Reini Urban
http://cpanel.net/ http://www.perl-compiler.org/

p5pRT · 2012-03-06T20:04:21Z

From @rurban

0001-sdbm.c-fix-off-by-one-access-to-global-.dir.patch

From 10e8459488fb0f1b63f50421cdec7d3a1de64691 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer
---
 ext/SDBM_File/sdbm/sdbm.c |   10 +++++-----
 ext/SDBM_File/sdbm/sdbm.h |    6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
  */
 	filelen = strlen(file);
 
-	if ((dirname = (char *) malloc(filelen + dirfext_len + 1
-				       + filelen + pagfext_len + 1)) == NULL)
+	if ((dirname = (char *) malloc(filelen + dirfext_len
+				       + filelen + pagfext_len)) == NULL)
 		return errno = ENOMEM, (DBM *) NULL;
 /*
  * build the file names
  */
 	memcpy(dirname, file, filelen);
-	memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
-	pagname = dirname + filelen + dirfext_len + 1;
+	memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+	pagname = dirname + filelen + dirfext_len;
 	memcpy(pagname, file, filelen);
-	memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+	memcpy(pagname + filelen, PAGFEXT, pagfext_len);
 
 	db = sdbm_prep(dirname, pagname, flags, mode);
 	free((char *) dirname);
diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
 #define SPLTMAX	10			/* maximum allowed splits */
 					/* for a single insertion */
 #ifdef VMS
-#define DIRFEXT	".sdbm_dir"
+#define DIRFEXT	".sdbm_dir\0"
 #else
-#define DIRFEXT	".dir"
+#define DIRFEXT	".dir\0"
 #endif
-#define PAGFEXT	".pag"
+#define PAGFEXT	".pag\0"
 
 typedef struct {
 	int dirf;		       /* directory file descriptor */
-- 
1.7.5.4

p5pRT · 2012-03-06T20:04:22Z

The RT System itself - Status changed from 'new' to 'open'

p5pRT · 2012-03-07T01:26:10Z

From @craigberry

From 657a387f083d6c36982ccecb1aa8df45426401ac Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer

Good catch. The bug came in here:

http://perl5.git.perl.org/perl.git/commit/081f72ad6fa2b76e0b3cd9046371b2dbd9130114

where we started calculating lengths with sizeof on string constants
instead of using strlen. Since string constants include the null
byte, sizeof(".dir"), for example, is 5, but we've been copying 6
bytes. We also allocate one extra byte, so the off-by-one is only on
what we read, not what we write, but it's still not good. I think you
know all that, but it wasn't in the bug report so I wrote up what I
realized in examining your patch.

ext/SDBM_File/sdbm/sdbm.c | 10 +++++-----
ext/SDBM_File/sdbm/sdbm.h | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
*/
filelen = strlen(file);

- if ((dirname = (char *) malloc(filelen + dirfext_len + 1
- + filelen + pagfext_len + 1)) == NULL)
+ if ((dirname = (char *) malloc(filelen + dirfext_len
+ + filelen + pagfext_len)) == NULL)
return errno = ENOMEM, (DBM *) NULL;
/*
* build the file names
*/
memcpy(dirname, file, filelen);
- memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
- pagname = dirname + filelen + dirfext_len + 1;
+ memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+ pagname = dirname + filelen + dirfext_len;
memcpy(pagname, file, filelen);
- memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+ memcpy(pagname + filelen, PAGFEXT, pagfext_len);

db = sdbm_prep(dirname, pagname, flags, mode);
free((char *) dirname);

That much looks good to me (except for the C++-style comment). I
might also be tempted to rename the "_len" variables to "_size" to
make clear that they represent allocated space, not string length.

diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
#define SPLTMAX 10 /* maximum allowed splits */
/* for a single insertion */
#ifdef VMS
-#define DIRFEXT ".sdbm_dir"
+#define DIRFEXT ".sdbm_dir\0"
#else
-#define DIRFEXT ".dir"
+#define DIRFEXT ".dir\0"
#endif
-#define PAGFEXT ".pag"
+#define PAGFEXT ".pag\0"

but I don't understand this at all. There is already a null byte at
the end of each of these constants; why do we need another?

So my revision of your patch would simply be the attached. Does that
make clang happy?

p5pRT · 2012-03-07T01:26:10Z

From @craigberry

111586_followup.patch

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..46be83e 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -78,8 +78,8 @@ sdbm_open(register char *file, register int flags, register int mode)
 	register char *dirname;
 	register char *pagname;
 	size_t filelen;
-	const size_t dirfext_len = sizeof(DIRFEXT "");
-	const size_t pagfext_len = sizeof(PAGFEXT "");
+	const size_t dirfext_size = sizeof(DIRFEXT "");
+	const size_t pagfext_size = sizeof(PAGFEXT "");
 
 	if (file == NULL || !*file)
 		return errno = EINVAL, (DBM *) NULL;
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
  */
 	filelen = strlen(file);
 
-	if ((dirname = (char *) malloc(filelen + dirfext_len + 1
-				       + filelen + pagfext_len + 1)) == NULL)
+	if ((dirname = (char *) malloc(filelen + dirfext_size
+				       + filelen + pagfext_size)) == NULL)
 		return errno = ENOMEM, (DBM *) NULL;
 /*
  * build the file names
  */
 	memcpy(dirname, file, filelen);
-	memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
-	pagname = dirname + filelen + dirfext_len + 1;
+	memcpy(dirname + filelen, DIRFEXT, dirfext_size);
+	pagname = dirname + filelen + dirfext_size;
 	memcpy(pagname, file, filelen);
-	memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+	memcpy(pagname + filelen, PAGFEXT, pagfext_size);
 
 	db = sdbm_prep(dirname, pagname, flags, mode);
 	free((char *) dirname);

p5pRT · 2012-03-08T13:08:48Z

From @rurban

Am 06.03.2012 um 19:26 schrieb Craig Berry via RT:

From 657a387f083d6c36982ccecb1aa8df45426401ac Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer

Good catch. The bug came in here:

http://perl5.git.perl.org/perl.git/commit/081f72ad6fa2b76e0b3cd9046371b2dbd9130114

where we started calculating lengths with sizeof on string constants
instead of using strlen. Since string constants include the null
byte, sizeof(".dir"), for example, is 5, but we've been copying 6
bytes. We also allocate one extra byte, so the off-by-one is only on
what we read, not what we write, but it's still not good. I think you
know all that, but it wasn't in the bug report so I wrote up what I
realized in examining your patch.
ext/SDBM_File/sdbm/sdbm.c | 10 +++++-----
ext/SDBM_File/sdbm/sdbm.h | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
*/
filelen = strlen(file);

- if ((dirname = (char *) malloc(filelen + dirfext_len + 1
- + filelen + pagfext_len + 1)) == NULL)
+ if ((dirname = (char *) malloc(filelen + dirfext_len
+ + filelen + pagfext_len)) == NULL)
return errno = ENOMEM, (DBM *) NULL;
/*
* build the file names
*/
memcpy(dirname, file, filelen);
- memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
- pagname = dirname + filelen + dirfext_len + 1;
+ memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+ pagname = dirname + filelen + dirfext_len;
memcpy(pagname, file, filelen);
- memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+ memcpy(pagname + filelen, PAGFEXT, pagfext_len);
   db = sdbm\_prep$dirname\, pagname\, flags\, mode$;
   free$\(char \*$ dirname\);
That much looks good to me (except for the C++-style comment).

Yeah, sorry. I forgot to clear that before doing git add.

I might also be tempted to rename the "_len" variables to "_size" to
make clear that they represent allocated space, not string length.

diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
#define SPLTMAX 10 /* maximum allowed splits */
/* for a single insertion */
#ifdef VMS
-#define DIRFEXT ".sdbm_dir"
+#define DIRFEXT ".sdbm_dir\0"
#else
-#define DIRFEXT ".dir"
+#define DIRFEXT ".dir\0"
#endif
-#define PAGFEXT ".pag"
+#define PAGFEXT ".pag\0"

but I don't understand this at all. There is already a null byte at
the end of each of these constants; why do we need another?

I know that string constants already include the final \0 but it made the code clearer to me.
I didn't hurt to have two, and I got rid of the all the additional +1 operations.
"Trading space for speed and clearness", just a matter of style.

So my revision of your patch would simply be the attached. Does that
make clang happy?

<111586_followup.patch>

I hope that I can test it today. But I'll be out of office from today noon for the next two weeks,
so I do not react please apply your version.

--
Reini

p5pRT · 2012-03-08T14:24:17Z

From @rurban

On Thu, Mar 8, 2012 at 7:07 AM, Reini Urban <rurban@cpanel.net> wrote:

Am 06.03.2012 um 19:26 schrieb Craig Berry via RT:

From 657a387f083d6c36982ccecb1aa8df45426401ac Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Tue, 6 Mar 2012 13:46:15 -0600
Subject: [PATCH] sdbm.c: fix off-by-one access to global ".dir"

Detected by clang -faddress-sanitizer

Good catch. The bug came in here:

http://perl5.git.perl.org/perl.git/commit/081f72ad6fa2b76e0b3cd9046371b2dbd9130114

where we started calculating lengths with sizeof on string constants
instead of using strlen. Since string constants include the null
byte, sizeof(".dir"), for example, is 5, but we've been copying 6
bytes. We also allocate one extra byte, so the off-by-one is only on
what we read, not what we write, but it's still not good. I think you
know all that, but it wasn't in the bug report so I wrote up what I
realized in examining your patch.

ext/SDBM_File/sdbm/sdbm.c | 10 +++++-----
ext/SDBM_File/sdbm/sdbm.h | 6 +++---
2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/ext/SDBM_File/sdbm/sdbm.c b/ext/SDBM_File/sdbm/sdbm.c
index c554e52..83f277f 100644
--- a/ext/SDBM_File/sdbm/sdbm.c
+++ b/ext/SDBM_File/sdbm/sdbm.c
@@ -88,17 +88,17 @@ sdbm_open(register char *file, register int flags, register int mode)
*/
filelen = strlen(file);

- if ((dirname = (char *) malloc(filelen + dirfext_len + 1
- + filelen + pagfext_len + 1)) == NULL)
+ if ((dirname = (char *) malloc(filelen + dirfext_len
+ + filelen + pagfext_len)) == NULL)
return errno = ENOMEM, (DBM *) NULL;
/*
* build the file names
*/
memcpy(dirname, file, filelen);
- memcpy(dirname + filelen, DIRFEXT, dirfext_len + 1);
- pagname = dirname + filelen + dirfext_len + 1;
+ memcpy(dirname + filelen, DIRFEXT, dirfext_len); // global-buffer-overflow - one right of ".dir"
+ pagname = dirname + filelen + dirfext_len;
memcpy(pagname, file, filelen);
- memcpy(pagname + filelen, PAGFEXT, pagfext_len + 1);
+ memcpy(pagname + filelen, PAGFEXT, pagfext_len);

db = sdbm_prep(dirname, pagname, flags, mode);
free((char *) dirname);

That much looks good to me (except for the C++-style comment).

Yeah, sorry. I forgot to clear that before doing git add.

I might also be tempted to rename the "_len" variables to "_size" to
make clear that they represent allocated space, not string length.

diff --git a/ext/SDBM_File/sdbm/sdbm.h b/ext/SDBM_File/sdbm/sdbm.h
index 2b8d0e9..37a4a26 100644
--- a/ext/SDBM_File/sdbm/sdbm.h
+++ b/ext/SDBM_File/sdbm/sdbm.h
@@ -10,11 +10,11 @@
#define SPLTMAX 10 /* maximum allowed splits */
/* for a single insertion */
#ifdef VMS
-#define DIRFEXT ".sdbm_dir"
+#define DIRFEXT ".sdbm_dir\0"
#else
-#define DIRFEXT ".dir"
+#define DIRFEXT ".dir\0"
#endif
-#define PAGFEXT ".pag"
+#define PAGFEXT ".pag\0"

but I don't understand this at all. There is already a null byte at
the end of each of these constants; why do we need another?

I know that string constants already include the final \0 but it made the code clearer to me.
I didn't hurt to have two, and I got rid of the all the additional +1 operations.
"Trading space for speed and clearness", just a matter of style.

So my revision of your patch would simply be the attached. Does that
make clang happy?

<111586_followup.patch>

I hope that I can test it today. But I'll be out of office from today noon for the next two weeks,
so I do not react please apply your version.

111586_followup.patch tested ok, GTG
--
Reini Urban
http://cpanel.net/ http://www.perl-compiler.org/

p5pRT · 2012-03-09T16:24:22Z

From @craigberry

On Thu, Mar 8, 2012 at 8:23 AM, Reini Urban <rurban@x-ray.at> wrote:

111586_followup.patch tested ok, GTG

Thanks, applied:

http://perl5.git.perl.org/perl.git/commitdiff/acdbe25bd91bf897e0cf373b91ab0814e21c4860

p5pRT · 2012-03-09T16:41:09Z

@cpansprout - Status changed from 'open' to 'resolved'

p5pRT closed this as completed Mar 9, 2012

p5pRT added Severity Critical distro-Linux hasPatch type-library labels Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH] `git log --pretty=format:%s HEAD^1..HEAD` #11988

[PATCH] `git log --pretty=format:%s HEAD^1..HEAD` #11988

p5pRT commented Mar 6, 2012

p5pRT commented Mar 6, 2012

p5pRT commented Mar 6, 2012

p5pRT commented Mar 6, 2012

p5pRT commented Mar 6, 2012

p5pRT commented Mar 7, 2012

p5pRT commented Mar 7, 2012

p5pRT commented Mar 8, 2012

p5pRT commented Mar 8, 2012

p5pRT commented Mar 9, 2012

p5pRT commented Mar 9, 2012

Navigation Menu

[PATCH] git log --pretty=format:%s HEAD^1..HEAD #11988

[PATCH] git log --pretty=format:%s HEAD^1..HEAD #11988

Comments

p5pRT commented Mar 6, 2012

p5pRT commented Mar 6, 2012

From @rurban

p5pRT commented Mar 6, 2012

From @rurban

p5pRT commented Mar 6, 2012

From @rurban

p5pRT commented Mar 6, 2012

p5pRT commented Mar 7, 2012

From @craigberry

p5pRT commented Mar 7, 2012

From @craigberry

p5pRT commented Mar 8, 2012

From @rurban

p5pRT commented Mar 8, 2012

From @rurban

p5pRT commented Mar 9, 2012

From @craigberry

p5pRT commented Mar 9, 2012

[PATCH] `git log --pretty=format:%s HEAD^1..HEAD` #11988

[PATCH] `git log --pretty=format:%s HEAD^1..HEAD` #11988