Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] attributes: fix memEQs shared #14072

Closed
p5pRT opened this issue Sep 4, 2014 · 9 comments

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Sep 4, 2014

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

Searchable as RT122701$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2014

From @rurban

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


attributes​: fix memEQs shared
 
Do not access memory buffers shorter than 6, and also check for
longer buffers starting with "shared". This bug caused an ASAN error
with my sv_grow performance improvement on short strings (commit 880c169,
reverted with 3c239be),
and went undetected because the shortest strings are 10 byte long.

This bug blocks sv_grow and future string comparison improvements.



Flags​:
  category=library
  severity=medium
  Type=Patch
  PatchStatus=HasPatch
  module=attributes


Site configuration information for perl 5.21.4​:

Configured by rurban at Mon Sep 1 04​:59​:49 CDT 2014.

Summary of my perl5 (revision 5 version 21 subversion 4) configuration​:
  Local Commit​: fe69a2bea4a56f064d3515845f0a08ee7b7b3fe3
  Ancestor​: f410e80
  Platform​:
  osname=linux, osvers=3.14-2-amd64, archname=x86_64-linux-debug-asan@​32818149
  uname='linux reini 3.14-2-amd64 #1 smp debian 3.14.13-2 (2014-07-24) x86_64 gnulinux '
  config_args='-de -Dusedevel -Uversiononly -Dinstallman1dir=none -Dinstallman3dir=none -Dinstallsiteman1dir=none -Dinstallsiteman3dir=none -DEBUGGING -Uuseithreads -D'cc=clang' -D'optimize='-fno-omit-frame-pointer -gline-tables-only'' -A'ccflags=-fsanitize=address' -A'ldflags=-fsanitize=address' -A'lddlflags=-shared\ -fsanitize=address' -Duseshrplib -Dcf_email=''rurban@​cpanel.net'' -Dperladmin=''rurban@​cpanel.net'' -Accflags=-Wno-unused-value'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  use64bitint=define, use64bitall=define, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='clang', ccflags ='-fsanitize=address -Wno-unused-value -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-fno-omit-frame-pointer -gline-tables-only',
  cppflags='-fsanitize=address -Wno-unused-value -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include'
  ccversion='', gccversion='4.2.1 Compatible Debian Clang 3.4.2 (tags/RELEASE_34/dot2-final)', gccosandvers=''
  intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
  ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='clang', ldflags =' -fsanitize=address -L/usr/local/lib'
  libpth=/usr/local/lib /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /lib64 /usr/lib64
  libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=libc-2.19.so, so=so, useshrplib=true, libperl=libperl.so
  gnulibc_version='2.19'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/lib/perl5/5.21.4/x86_64-linux-debug-asan@​32818149/CORE'
  cccdlflags='-fPIC', lddlflags=' -shared -fsanitize=address -L/usr/local/lib '

Locally applied patches​:
  32818149be28edd2a58ea345eea388bf0d3dc329
  fe69a2bea4a56f064d3515845f0a08ee7b7b3fe3


@​INC for perl 5.21.4​:
  /usr/local/lib/perl5/site_perl/5.21.4/x86_64-linux-debug-asan@​32818149
  /usr/local/lib/perl5/site_perl/5.21.4
  /usr/local/lib/perl5/5.21.4/x86_64-linux-debug-asan@​32818149
  /usr/local/lib/perl5/5.21.4
  /usr/local/lib/perl5/site_perl/5.21.3
  /usr/local/lib/perl5/site_perl/5.21.2
  /usr/local/lib/perl5/site_perl/5.21.1
  /usr/local/lib/perl5/site_perl/5.21.0
  /usr/local/lib/perl5/site_perl/5.20.1
  /usr/local/lib/perl5/site_perl/5.20.0
  /usr/local/lib/perl5/site_perl/5.19.11
  /usr/local/lib/perl5/site_perl/5.19.10
  /usr/local/lib/perl5/site_perl/5.19.9
  /usr/local/lib/perl5/site_perl/5.19.8
  /usr/local/lib/perl5/site_perl/5.19.7
  /usr/local/lib/perl5/site_perl/5.19.6
  /usr/local/lib/perl5/site_perl/5.19.5
  /usr/local/lib/perl5/site_perl/5.19.4
  /usr/local/lib/perl5/site_perl/5.19.3
  /usr/local/lib/perl5/site_perl/5.19.2
  /usr/local/lib/perl5/site_perl/5.19.1
  /usr/local/lib/perl5/site_perl/5.19.0
  /usr/local/lib/perl5/site_perl/5.18.2
  /usr/local/lib/perl5/site_perl/5.18.1
  /usr/local/lib/perl5/site_perl/5.18.0
  /usr/local/lib/perl5/site_perl/5.17.11
  /usr/local/lib/perl5/site_perl/5.17.10
  /usr/local/lib/perl5/site_perl/5.17.8
  /usr/local/lib/perl5/site_perl/5.17.7
  /usr/local/lib/perl5/site_perl/5.17.6
  /usr/local/lib/perl5/site_perl/5.17.5
  /usr/local/lib/perl5/site_perl/5.17.4
  /usr/local/lib/perl5/site_perl/5.17.3
  /usr/local/lib/perl5/site_perl/5.17.2
  /usr/local/lib/perl5/site_perl/5.17.1
  /usr/local/lib/perl5/site_perl/5.17.0
  /usr/local/lib/perl5/site_perl/5.17
  /usr/local/lib/perl5/site_perl/5.16.3
  /usr/local/lib/perl5/site_perl/5.16.2
  /usr/local/lib/perl5/site_perl/5.16.1
  /usr/local/lib/perl5/site_perl/5.16.0
  /usr/local/lib/perl5/site_perl/5.15.9
  /usr/local/lib/perl5/site_perl/5.15.8
  /usr/local/lib/perl5/site_perl/5.15.7
  /usr/local/lib/perl5/site_perl/5.15.6
  /usr/local/lib/perl5/site_perl/5.15.5
  /usr/local/lib/perl5/site_perl/5.15.4
  /usr/local/lib/perl5/site_perl/5.15.3
  /usr/local/lib/perl5/site_perl/5.15.2
  /usr/local/lib/perl5/site_perl/5.14.4
  /usr/local/lib/perl5/site_perl/5.14.3
  /usr/local/lib/perl5/site_perl/5.14.2
  /usr/local/lib/perl5/site_perl/5.14.1
  /usr/local/lib/perl5/site_perl/5.12.5
  /usr/local/lib/perl5/site_perl/5.12.4
  /usr/local/lib/perl5/site_perl/5.10.1
  /usr/local/lib/perl5/site_perl/5.8.9
  /usr/local/lib/perl5/site_perl/5.8.8
  /usr/local/lib/perl5/site_perl/5.8.7
  /usr/local/lib/perl5/site_perl/5.8.6
  /usr/local/lib/perl5/site_perl/5.8.5
  /usr/local/lib/perl5/site_perl/5.8.4
  /usr/local/lib/perl5/site_perl/5.8.3
  /usr/local/lib/perl5/site_perl/5.8.2
  /usr/local/lib/perl5/site_perl/5.8.1
  /usr/local/lib/perl5/site_perl/5.6.2
  /usr/local/lib/perl5/site_perl
  .


Environment for perl 5.21.4​:
  HOME=/home/rurban
  LANG=en_US.utf8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/home/rurban/bin​:/usr/local/bin​:/usr/bin​:/bin​:/usr/local/games​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2014

From @rurban

0001-attributes-fix-memEQs-shared.patch
From 776476c29f4b1a645837d5e6193c1c42710d0c2d Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Thu, 4 Sep 2014 09:49:15 -0500
Subject: [PATCH] attributes: fix memEQs shared

Do not access memory buffers shorter than 6, and also check for
longer buffers starting with "shared". This bug caused an ASAN error with
my sv_grow performance improvement on short strings (commit 880c169,
reverted with 3c239be),
and went undetected because the shortest strings are 10 byte long.
---
 ext/attributes/attributes.xs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git ext/attributes/attributes.xs ext/attributes/attributes.xs
index dbb644d..2238050 100644
--- ext/attributes/attributes.xs
+++ ext/attributes/attributes.xs
@@ -97,11 +97,11 @@ modify_SV_attributes(pTHX_ SV *sv, SV **retlist, SV **attrlist, int numattrs)
 	    }
 	    break;
 	default:
-	    if (memEQs(name, 6, "shared")) {
-			if (negated)
-			    Perl_croak(aTHX_ "A variable may not be unshared");
-			SvSHARE(sv);
-                        continue;
+	    if (len == 6 && memEQs(name, 6, "shared")) {
+                if (negated)
+                    Perl_croak(aTHX_ "A variable may not be unshared");
+                SvSHARE(sv);
+                continue;
 	    }
 	    break;
 	}
-- 
2.1.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2014

From @doughera88

On Thu, Sep 04, 2014 at 07​:57​:21AM -0700, rurban @​ cpanel . net wrote​:

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

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

-----------------------------------------------------------------
attributes​: fix memEQs shared

Do not access memory buffers shorter than 6, and also check for
longer buffers starting with "shared". This bug caused an ASAN error
with my sv_grow performance improvement on short strings (commit 880c169,
reverted with 3c239be),
and went undetected because the shortest strings are 10 byte long.

This bug blocks sv_grow and future string comparison improvements.

-----------------------------------------------------------------

Thanks for the quick and accurate diagnosis and fix. I've applied
a simplified version of your patch in gdd36996. (The memEQs macro
does the length calculations for you, if you call it with 'len' instead
of the hardcoded '6'.) Also, t/porting/cmp_versions.t wasn't happy
until I bumped the version number in attributes.pm.

Thanks,

--
  Andy Dougherty doughera@​lafayette.edu

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2014

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

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 4, 2014

From @rurban

On 09/04/2014 11​:47 AM, Andy Dougherty via RT wrote​:

On Thu, Sep 04, 2014 at 07​:57​:21AM -0700, rurban @​ cpanel . net wrote​:

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

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

-----------------------------------------------------------------
attributes​: fix memEQs shared

Do not access memory buffers shorter than 6, and also check for
longer buffers starting with "shared". This bug caused an ASAN error
with my sv_grow performance improvement on short strings (commit 880c169,
reverted with 3c239be),
and went undetected because the shortest strings are 10 byte long.

This bug blocks sv_grow and future string comparison improvements.

-----------------------------------------------------------------

Thanks for the quick and accurate diagnosis and fix. I've applied
a simplified version of your patch in gdd36996. (The memEQs macro
does the length calculations for you, if you call it with 'len' instead
of the hardcoded '6'.) Also, t/porting/cmp_versions.t wasn't happy
until I bumped the version number in attributes.pm.

I see.
I wondered about the t/porting/cmp_versions.t fail also, but didn't check.

Thanks!

In a subsequent patch I want to change min. SvLEN to wordsize (4|8) with
calloc of the first word only, and then in memEQ check the first word,
and then doing the memcmp of the rest if that fails.

Should be much faster I hope. It was much faster in my tests because we
don't need to setup the loop if the string is short. Numbers at
http​://blogs.perl.org/users/rurban/2014/08/perfect-hashes-and-faster-than-memcmp.html

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 8, 2014

From @bulk88

On Thu Sep 04 09​:47​:40 2014, doughera wrote​:

Thanks for the quick and accurate diagnosis and fix. I've applied
a simplified version of your patch in gdd36996. (The memEQs macro

gdd36996 isnt a known commit

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 8, 2014

From @Tux

On Mon, 8 Sep 2014 06​:33​:34 -0700, "bulk88 via RT"
<perlbug-followup@​perl.org> wrote​:

On Thu Sep 04 09​:47​:40 2014, doughera wrote​:

Thanks for the quick and accurate diagnosis and fix. I've applied
a simplified version of your patch in gdd36996. (The memEQs macro

gdd36996 isnt a known commit

Drop the leading g

$ git show dd36996
commit dd36996
Author​: Andy Dougherty <doughera@​lafayette.edu>
Date​: Thu Sep 4 12​:24​:42 2014 -0400

  Correct usage of memEQs in attributes.xs [perl #122701]

  Reported and diagnosed by Reini Urban <rurban@​cpanel.net>. The call to
  memEQs(name, 6, "shared") could fail if name were shorter than 6 bytes,
  or if name were longer than 6, but started with "shared".

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 8, 2014

From @rurban

On 09/08/2014 08​:33 AM, bulk88 via RT wrote​:

On Thu Sep 04 09​:47​:40 2014, doughera wrote​:

Thanks for the quick and accurate diagnosis and fix. I've applied
a simplified version of your patch in gdd36996. (The memEQs macro

gdd36996 isnt a known commit

It is known.

The first char specifies the version control software (g for git, s for
a potential svn, p for a potential perforce, ...), the rest specifies
the commit in question.

$ git describe
v5.21.3-448-gdd36996

@p5pRT p5pRT closed this Sep 28, 2014
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 28, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.