runtime error: null pointer passed as argument 1, which is declared to never be null (pp_hot.c:4147:6) #16079
Comments
From @geeknikWhile compiling 1e629c2 for the purposes of fuzzing, I encountered some Command line: Error:
I tried to get a stack trace, but this is all UBSan would give up: SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior pp_hot.c:4147:6 in |
From @tonycozOn Wed, Jul 12, 2017 at 11:51:52AM -0700, Brian Carpenter wrote:
I haven't been able to reproduce this. To get more information you might try it under a debugger, so ASAN_OPTIONS=abort_on_error=1 gdb --args ./miniperl -Ilib make_ext.pl cpan/Archive-Tar/pm_to_blib MAKE="make" LIBPERL_A=libperl.a Given the line number, this code: Copy(MARK+1,AvARRAY(av),items,SV*); is the problem, but I don't see how MARK can be NULL (and MARK+1 won't So it's likely AvARRAY(av) is NULL, and in this case items is probably Copy() is essentially a wrapper around memcpy(), and from this
it looks like memcpy() has NULL restrictions on its pointer parameters https://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0 and https://www.imperialviolet.org/2016/06/26/nonnull.html make a good argument that it isn't. Can you reproduce the problem if you add a conditional to that line, if (items) Copy(MARK+1,AvARRAY(av),items,SV*); Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @geeknikllvm-symbolizer wasn't in my $PATH on this particular machine. Once I afl-clang-fast [tpcg] 2.46b by <lszekeres@google.com>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior pp_hot.c:4147:6 in |
From @tonycozOn Wed, 12 Jul 2017 22:56:09 -0700, brian.carpenter@gmail.com wrote:
Thanks, I'm pretty sure the fix in my previous response will solve it, could you please try it? Thanks, |
From @geeknikSorry for the delay, your change does indeed solve the problem I was having and I was able to compile perl v5.27.2-135-g7aaa36b196* without this particular UBSan message. On Thu, 13 Jul 2017 17:49:55 -0700, tonyc wrote:
|
From @tonycozOn Sun, 13 Aug 2017 14:58:16 -0700, brian.carpenter@gmail.com wrote:
The attached should fix both this and #131892. Please let me know if it does. Tony |
From @tonycoz0001-perl-131746-avoid-undefined-behaviour-in-Copy-etc.patchFrom 4f049cb3136c9916ededf0416d6439a4838ae7c8 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 14 Aug 2017 11:52:39 +1000
Subject: (perl #131746) avoid undefined behaviour in Copy() etc
These functions depend on C library functions which have undefined
behaviour when passed NULL pointers, even when passed a zero 'n' value.
Some compilers use this information, ie. assume the pointers are
non-NULL when optimizing any following code, so we do need to
prevent such unguarded calls.
My initial thought was to add conditionals to each macro to skip the
call to the library function when n is zero, but this adds a cost to
every use of these macros, even when the n value is always true.
So instead I added asserts() which will give us a much more visible
indicator of such broken code and revealed the pp_caller and Glob.xs
issues also patched here.
---
ext/File-Glob/Glob.pm | 2 +-
ext/File-Glob/Glob.xs | 2 +-
handy.h | 14 +++++++-------
pp_ctl.c | 3 ++-
pp_hot.c | 3 ++-
5 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/ext/File-Glob/Glob.pm b/ext/File-Glob/Glob.pm
index 4f74023..6614e82 100644
--- a/ext/File-Glob/Glob.pm
+++ b/ext/File-Glob/Glob.pm
@@ -37,7 +37,7 @@ pop @{$EXPORT_TAGS{bsd_glob}}; # no "glob"
@EXPORT_OK = (@{$EXPORT_TAGS{'glob'}}, 'csh_glob');
-$VERSION = '1.29';
+$VERSION = '1.30';
sub import {
require Exporter;
diff --git a/ext/File-Glob/Glob.xs b/ext/File-Glob/Glob.xs
index e0a3681..9779d54 100644
--- a/ext/File-Glob/Glob.xs
+++ b/ext/File-Glob/Glob.xs
@@ -121,7 +121,7 @@ iterate(pTHX_ bool(*globber)(pTHX_ AV *entries, const char *pat, STRLEN len, boo
/* chuck it all out, quick or slow */
if (gimme == G_ARRAY) {
- if (!on_stack) {
+ if (!on_stack && AvFILLp(entries) + 1) {
EXTEND(SP, AvFILLp(entries)+1);
Copy(AvARRAY(entries), SP+1, AvFILLp(entries)+1, SV *);
SP += AvFILLp(entries)+1;
diff --git a/handy.h b/handy.h
index c3848bf..7ef7e25 100644
--- a/handy.h
+++ b/handy.h
@@ -2409,17 +2409,17 @@ void Perl_mem_log_del_sv(const SV *sv, const char *filename, const int linenumbe
#define Safefree(d) safefree(MEM_LOG_FREE((Malloc_t)(d)))
#endif
-#define Move(s,d,n,t) (MEM_WRAP_CHECK_(n,t) (void)memmove((char*)(d),(const char*)(s), (n) * sizeof(t)))
-#define Copy(s,d,n,t) (MEM_WRAP_CHECK_(n,t) (void)memcpy((char*)(d),(const char*)(s), (n) * sizeof(t)))
-#define Zero(d,n,t) (MEM_WRAP_CHECK_(n,t) (void)memzero((char*)(d), (n) * sizeof(t)))
+#define Move(s,d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), assert(s), (void)memmove((char*)(d),(const char*)(s), (n) * sizeof(t)))
+#define Copy(s,d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), assert(s), (void)memcpy((char*)(d),(const char*)(s), (n) * sizeof(t)))
+#define Zero(d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), (void)memzero((char*)(d), (n) * sizeof(t)))
-#define MoveD(s,d,n,t) (MEM_WRAP_CHECK_(n,t) memmove((char*)(d),(const char*)(s), (n) * sizeof(t)))
-#define CopyD(s,d,n,t) (MEM_WRAP_CHECK_(n,t) memcpy((char*)(d),(const char*)(s), (n) * sizeof(t)))
+#define MoveD(s,d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), assert(s), memmove((char*)(d),(const char*)(s), (n) * sizeof(t)))
+#define CopyD(s,d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), assert(s), memcpy((char*)(d),(const char*)(s), (n) * sizeof(t)))
#ifdef HAS_MEMSET
-#define ZeroD(d,n,t) (MEM_WRAP_CHECK_(n,t) memzero((char*)(d), (n) * sizeof(t)))
+#define ZeroD(d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), memzero((char*)(d), (n) * sizeof(t)))
#else
/* Using bzero(), which returns void. */
-#define ZeroD(d,n,t) (MEM_WRAP_CHECK_(n,t) memzero((char*)(d), (n) * sizeof(t)),d)
+#define ZeroD(d,n,t) (MEM_WRAP_CHECK_(n,t) assert(d), memzero((char*)(d), (n) * sizeof(t)),d)
#endif
#define PoisonWith(d,n,t,b) (MEM_WRAP_CHECK_(n,t) (void)memset((char*)(d), (U8)(b), (n) * sizeof(t)))
diff --git a/pp_ctl.c b/pp_ctl.c
index f91bb4d..3e8c7b4 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -1997,7 +1997,8 @@ PP(pp_caller)
if (AvMAX(PL_dbargs) < AvFILLp(ary) + off)
av_extend(PL_dbargs, AvFILLp(ary) + off);
- Copy(AvALLOC(ary), AvARRAY(PL_dbargs), AvFILLp(ary) + 1 + off, SV*);
+ if (AvFILLp(ary) + 1 + off)
+ Copy(AvALLOC(ary), AvARRAY(PL_dbargs), AvFILLp(ary) + 1 + off, SV*);
AvFILLp(PL_dbargs) = AvFILLp(ary) + off;
}
mPUSHi(CopHINTS_get(cx->blk_oldcop));
diff --git a/pp_hot.c b/pp_hot.c
index 528817f..95bdf54 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -4333,7 +4333,8 @@ PP(pp_entersub)
AvARRAY(av) = ary;
}
- Copy(MARK+1,AvARRAY(av),items,SV*);
+ if (items)
+ Copy(MARK+1,AvARRAY(av),items,SV*);
AvFILLp(av) = items - 1;
}
if (UNLIKELY((cx->blk_u16 & OPpENTERSUB_LVAL_MASK) == OPpLVAL_INTRO &&
--
2.1.4
|
From @geeknikLGTM On Sun, 13 Aug 2017 18:54:27 -0700, tonyc wrote:
|
|
@tonycoz - Status changed from 'open' to 'pending release' |
From @iabynOn Sun, Sep 03, 2017 at 10:08:55PM -0700, Tony Cook via RT wrote:
The assert shows up a problem in threads.xs which was breaking threaded commit 97fcda7 threads.xs: don't Copy() null pointer -- |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#131746 (status was 'resolved')
Searchable as RT131746$
The text was updated successfully, but these errors were encountered: