[Review] All NN-argument functions potentially affected by GCC 4.8+ -O2-level mis-optimization

[p5pRT]

Migrated from rt.perl.org#125570 (status was 'open')

Searchable as RT125570$

[p5pRT]

From @ribasushi

As of version 4.8 GCC under -O2 compiles away all blocks of the form​:

if (! \<\< argument marked with __attribute__((nonnull(1))) >> ) { ... }

This does not happen under -O1. It is not clear which (if any) -fno* flag controls this. The basic mechanism is described here[1]\, relevant gcc "wontfix" bugreports are here[2] and here[3] (fished from this[4] rant).

Currently the only *known* manifestation of this is perl 5.10.0 segfaulting in Perl_parser_dup() as this entire return() block is lost[5] (due to P_p_d marked as NN here[6]).

*However*\, given that the current perl source is full of NN-marked arguments\, and just after a cursory search I can find several return-statements similar to [5] within blead \, I am raising this to the sec-list\, so someone can actually audit our use of nonnull.

I had a short chat with Nicholas\, who also recommended I contact p5-sec​:

\ I'd suggest mailing p5-sec\, because that creates an RT ticket. \ if "we" think that it's not an "OMG panic" job\, it's trivial to move the ticket to the public queue \ the GCC NN stuff is really bloody annoying because you can't add a "not null" annotation to variables or structure elements \ *just* function pointers \ but our plan was to have assertions everywhere which check the not-NULL-ness \ (clearly assertions only exist on certain debugging builds)

Please let me know whether this is deemed high- or low-risk\, as I would like to proceed further towards a patch for Devel​::PatchPerl.

Cheers

[1] http​://rachid.koucha.free.fr/tech_corner/nonnull_gcc_attribute.html [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36166 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 [4] http​://l.longi.li/blog/2010/04/19/gcc-s-attribute-nonnull-not-helpful-at-all/ [5] https://metacpan.org/source/RGARCIA/perl-5.10.0/sv.c#L9567-9568 [6] https://metacpan.org/source/RGARCIA/perl-5.10.0/embed.fnc#L1094

[p5pRT]

From @nwc10

On Tue\, Jul 07\, 2015 at 08​:57​:44AM -0700\, Peter Rabbitson wrote​:

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

As of version 4.8 GCC under -O2 compiles away all blocks of the form​:

if (! \<\< argument marked with __attribute__((nonnull(1))) >> ) { ... }

This does not happen under -O1. It is not clear which (if any) -fno* flag controls this. The basic mechanism is described here[1]\, relevant gcc "wontfix" bugreports are here[2] and here[3] (fished from this[4] rant).

Currently the only *known* manifestation of this is perl 5.10.0 segfaulting in Perl_parser_dup() as this entire return() block is lost[5] (due to P_p_d marked as NN here[6]).

Sorry\, I failed to ask "this is for blead" and assumed that it was.

[I'm on holiday and don't have much time. Right now I have a little as my son is very happily busy playing a memory game on the tablet. Key part being that in *this* instance\, every pair you need to match is some sort of digger. :-)]

*However*\, given that the current perl source is full of NN-marked arguments\, and just after a cursory search I can find several return-statements similar to [5] within blead \, I am raising this to the sec-list\, so someone can actually audit our use of nonnull.

I had a short chat with Nicholas\, who also recommended I contact p5-sec​:

\ I'd suggest mailing p5-sec\, because that creates an RT ticket. \ if "we" think that it's not an "OMG panic" job\, it's trivial to move the ticket to the public queue \ the GCC NN stuff is really bloody annoying because you can't add a "not null" annotation to variables or structure elements \ *just* function pointers \ but our plan was to have assertions everywhere which check the not-NULL-ness \ (clearly assertions only exist on certain debugging builds)

I didn't remember when the assertions were added\, but clearly it's post 5.10.0

Its code\, as you linked to\, is this​:

yy_parser * Perl_parser_dup(pTHX_ const yy_parser *proto\, CLONE_PARAMS* param) {   yy_parser *parser;

  if (!proto)   return NULL;

and yes\, things like this are exactly the SEGV minefield you correctly inferred. However\, the code now is *this*​:

yy_parser * Perl_parser_dup(pTHX_ const yy_parser *const proto\, CLONE_PARAMS *const param) {   yy_parser *parser;

  PERL_ARGS_ASSERT_PARSER_DUP;

where that macro is this​:

#define PERL_ARGS_ASSERT_PARSER_DUP \   assert(param)

(the macros I mentioned on IRC)

and those assert()s in macros have picked up all places where the "not NULL" declaration was bogus (and we fixed them)

They were added (in bulk) by this commit​:

commit 7918f24d20384771923d344a382e1d16d9552018 Author​: Nicholas Clark \nick@&#8203;ccl4\.org Date​: Tue Feb 12 13​:15​:20 2008 +0000

  assert() that every NN argument is not NULL. Otherwise we have the   ability to create landmines that will explode under someone in the   future when they upgrade their compiler to one with better   optimisation. We've already done this at least twice.   (Yes\, some of the assertions are after code that would already have   SEGVd because it already deferences a pointer\, but they are put in   to make it easier to automate checking that each and every case is   covered.)   Add a tool\, checkARGS_ASSERT.pl\, to check that every case is covered.  
  p4raw-id​: //depot/perl@​33291

42 files changed\, 4006 insertions(+)\, 85 deletions(-)

which is merged to v5.10.1

and I'd infer that I only added it after I fixed everything that it found\, such as

commit cf684db655607eaaa00d57bcd72585cbdd05a7b4 Author​: Nicholas Clark \nick@&#8203;ccl4\.org Date​: Mon Feb 11 19​:22​:18 2008 +0000

  In Perl_sv_catpv()\, Perl_sv_catpv_mg() the ptr can be not NULL.   In Perl_sv_inc() and Perl_sv_dec()\, the sv can be not NULL.   In Perl_parser_dup() the proto parser can be NULL.   In Perl_ptr_table_find()\, the sought-for pointer can be NULL.   In Perl_save_set_svflags()\, the saved SV can't be NULL.  
  p4raw-id​: //depot/perl@​33283

(that includes the specific case that you hit)

IIRC in maint-5.8 whilst most of the "not NULL" framework is there (to keep merging easy)\, embed.pl has the line to add the "not NULL" attribute disabled\, so it's never presented to the compiler.

Hence\, I think that

1) the only release at broad systematic risk is v5.10.0 2) which is already obsoleted by a fixed version\, v5.10.1   so the specific problems were fixed within the security support window

3) we have the systematic risk mitigated   (I'm not aware of anything being found in the past 5 years)

So\, unless (and until) we can find problems caused by "not NULL" on anything released in the past 5 years\, I don't think that there's an active bug here.

Nicholas Clark

[p5pRT]

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

[p5pRT]

From @jhi

Given all the pitfalls with the nonnull attribute (ranging from misleading and confusing to outright dangerous) could someone remind me of the benefits?

Off-hand... Mostly it seems to just give permission for optimizers to do surprising things. Exploiting undefined behaviour by "this cannot happen so let's remove it". It certainly doesn't magically make code "NULL-resistant".

If we only need to generating our own assertions\, maybe we instead should use some attribute of our own?

On Jul 8\, 2015\, at 10​:41\, Nicholas Clark \nick@&#8203;ccl4\.org wrote​:

On Tue\, Jul 07\, 2015 at 08​:57​:44AM -0700\, Peter Rabbitson wrote​: # New Ticket Created by Peter Rabbitson # Please include the string​: [perl #125570] # in the subject line of all future correspondence about this issue. # \<URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=125570 >

As of version 4.8 GCC under -O2 compiles away all blocks of the form​:

if (! \<\< argument marked with __attribute__((nonnull(1))) >> ) { ... }

This does not happen under -O1. It is not clear which (if any) -fno* flag controls this. The basic mechanism is described here[1]\, relevant gcc "wontfix" bugreports are here[2] and here[3] (fished from this[4] rant).

Currently the only *known* manifestation of this is perl 5.10.0 segfaulting in Perl_parser_dup() as this entire return() block is lost[5] (due to P_p_d marked as NN here[6]).

Sorry\, I failed to ask "this is for blead" and assumed that it was.

[I'm on holiday and don't have much time. Right now I have a little as my son is very happily busy playing a memory game on the tablet. Key part being that in *this* instance\, every pair you need to match is some sort of digger. :-)]

*However*\, given that the current perl source is full of NN-marked arguments\, and just after a cursory search I can find several return-statements similar to [5] within blead \, I am raising this to the sec-list\, so someone can actually audit our use of nonnull.

I had a short chat with Nicholas\, who also recommended I contact p5-sec​:

\ I'd suggest mailing p5-sec\, because that creates an RT ticket. \ if "we" think that it's not an "OMG panic" job\, it's trivial to move the ticket to the public queue \ the GCC NN stuff is really bloody annoying because you can't add a "not null" annotation to variables or structure elements \ *just* function pointers \ but our plan was to have assertions everywhere which check the not-NULL-ness \ (clearly assertions only exist on certain debugging builds)

I didn't remember when the assertions were added\, but clearly it's post 5.10.0

Its code\, as you linked to\, is this​:

yy_parser * Perl_parser_dup(pTHX_ const yy_parser *proto\, CLONE_PARAMS* param) { yy_parser *parser;

if (!proto) return NULL;

and yes\, things like this are exactly the SEGV minefield you correctly inferred. However\, the code now is *this*​:

yy_parser * Perl_parser_dup(pTHX_ const yy_parser *const proto\, CLONE_PARAMS *const param) { yy_parser *parser;

PERL_ARGS_ASSERT_PARSER_DUP;

where that macro is this​:

#define PERL_ARGS_ASSERT_PARSER_DUP \ assert(param)

(the macros I mentioned on IRC)

and those assert()s in macros have picked up all places where the "not NULL" declaration was bogus (and we fixed them)

They were added (in bulk) by this commit​:

commit 7918f24d20384771923d344a382e1d16d9552018 Author​: Nicholas Clark \nick@&#8203;ccl4\.org Date​: Tue Feb 12 13​:15​:20 2008 +0000

assert() that every NN argument is not NULL. Otherwise we have the ability to create landmines that will explode under someone in the future when they upgrade their compiler to one with better optimisation. We've already done this at least twice. (Yes\, some of the assertions are after code that would already have SEGVd because it already deferences a pointer\, but they are put in to make it easier to automate checking that each and every case is covered.) Add a tool\, checkARGS_ASSERT.pl\, to check that every case is covered.

p4raw-id​: //depot/perl@​33291

42 files changed\, 4006 insertions(+)\, 85 deletions(-)

which is merged to v5.10.1

and I'd infer that I only added it after I fixed everything that it found\, such as

commit cf684db655607eaaa00d57bcd72585cbdd05a7b4 Author​: Nicholas Clark \nick@&#8203;ccl4\.org Date​: Mon Feb 11 19​:22​:18 2008 +0000

In Perl_sv_catpv()\, Perl_sv_catpv_mg() the ptr can be not NULL. In Perl_sv_inc() and Perl_sv_dec()\, the sv can be not NULL. In Perl_parser_dup() the proto parser can be NULL. In Perl_ptr_table_find()\, the sought-for pointer can be NULL. In Perl_save_set_svflags()\, the saved SV can't be NULL.

p4raw-id​: //depot/perl@​33283

(that includes the specific case that you hit)

IIRC in maint-5.8 whilst most of the "not NULL" framework is there (to keep merging easy)\, embed.pl has the line to add the "not NULL" attribute disabled\, so it's never presented to the compiler.

Hence\, I think that

1) the only release at broad systematic risk is v5.10.0 2) which is already obsoleted by a fixed version\, v5.10.1 so the specific problems were fixed within the security support window

3) we have the systematic risk mitigated (I'm not aware of anything being found in the past 5 years)

So\, unless (and until) we can find problems caused by "not NULL" on anything released in the past 5 years\, I don't think that there's an active bug here.

Nicholas Clark

[p5pRT]

From @iabyn

On Thu\, Jul 09\, 2015 at 11​:47​:11AM +0300\, Jarkko Hietaniemi wrote​:

Given all the pitfalls with the nonnull attribute (ranging from misleading and confusing to outright dangerous) could someone remind me of the benefits?

Off-hand... Mostly it seems to just give permission for optimizers to do surprising things. Exploiting undefined behaviour by "this cannot happen so let's remove it". It certainly doesn't magically make code "NULL-resistant".

If we only need to generating our own assertions\, maybe we instead should use some attribute of our own?

I think perhaps two things are getting conflated here by us and/or gcc​:

1) telling the compiler that we *know* that function X will never be called with a null argument N (because reasons) and that therefore it's safe to optimise with that assumption - similar to the way we say that we *know* that certain die-like functions can never return;

2) telling the run-time that this function *shouldn't* be called with a null arg\, and to helpfully abort if it does (on debugging builds anyway).

I think the PERL_ARGS_ASSERT_* are doing (2)\, while the __attribute__((nonnull(2))) is for (1).

The NN info we add to embed.fnc is for (2)\, so it's probably wrong for us to generate the __attribute__ in addition to the ASSERT. If so\, it may have been wrong with the recent removal of various "if (!p) croak()" at the starts of functions. Perhaps PERL_ARGS_ASSERT_* on non-debugging fields should automatically inject lots of 'if (!p) croak("panic​: ..")' statements instead?

-- You're only as old as you look.

[p5pRT]

From @Leont

On Thu\, Jul 9\, 2015 at 4​:01 PM\, Dave Mitchell \davem@&#8203;iabyn\.com wrote​:

On Thu\, Jul 09\, 2015 at 11​:47​:11AM +0300\, Jarkko Hietaniemi wrote​:

Given all the pitfalls with the nonnull attribute (ranging from misleading and confusing to outright dangerous) could someone remind me of the benefits?

Off-hand... Mostly it seems to just give permission for optimizers to do surprising things. Exploiting undefined behaviour by "this cannot happen so let's remove it". It certainly doesn't magically make code "NULL-resistant".

If we only need to generating our own assertions\, maybe we instead should use some attribute of our own?

I think perhaps two things are getting conflated here by us and/or gcc​:

1) telling the compiler that we *know* that function X will never be called with a null argument N (because reasons) and that therefore it's safe to optimise with that assumption - similar to the way we say that we *know* that certain die-like functions can never return;

2) telling the run-time that this function *shouldn't* be called with a null arg\, and to helpfully abort if it does (on debugging builds anyway).

I think the PERL_ARGS_ASSERT_* are doing (2)\, while the __attribute__((nonnull(2))) is for (1).

Exactly.

The NN info we add to embed.fnc is for (2)\, so it's probably wrong for us to generate the __attribute__ in addition to the ASSERT.

Yeah\, that may not be very helpful.

If so\, it may have been wrong with the recent removal of various "if (!p) croak()" at the starts of functions. Perhaps PERL_ARGS_ASSERT_* on non-debugging fields should automatically inject lots of 'if (!p) croak("panic​: ..")' statements instead?

If I understand things correctly\, those were defensive programming. We shouldn't really need them\, and they shouldn't (famous last words) ever trigger anyway. I suspect that generally speaking having them only in debugging perls may be a reasonable trade-off.

Leon

[p5pRT]

From @ribasushi

On 07/08/2015 09​:42 AM\, Nicholas Clark via RT wrote​:

So\, unless (and until) we can find problems caused by "not NULL" on anything released in the past 5 years\, I don't think that there's an active bug here.

If there is rough agreement on the above "nothing to see here" and/or there are no immediate plans to change anything\, I would like to ask for the ticket to be moved to the public perl5 RT queue.

Thanks!

[p5pRT]

From @rjbs

* Peter Rabbitson \rabbit@&#8203;rabbit\.us [2015-07-15T07​:08​:49]

If there is rough agreement on the above "nothing to see here" and/or there are no immediate plans to change anything\, I would like to ask for the ticket to be moved to the public perl5 RT queue.

If there is no objection to this by Monday\, I will do this... but on Monday I will be on vacation\, so what I really mean is "I will do this no sooner than Monday\, probably Monday\, and maybe a few days after if I'm having too nice a time."

-- rjbs

[p5pRT]

From @rjbs

This ticket has been moved to the perl5 queue.

-- rjbs

[p5pRT]

From [Unknown Contact. See original ticket]

This ticket has been moved to the perl5 queue.

-- rjbs

[p5pRT]

From @bulk88

I know the Win32 GCC build\, with DEBUGGING on\, is -O2\, IDK why http​://perl5.git.perl.org/perl.git/commitdiff/99efab1281ccea6f7df2a4d0affc5479291e2350 . So the assert for NULL will be optimized away.

In hints/linux.sh I see

---

case "$optimize" in # use -O2 by default ; -O3 doesn't seem to bring significant benefits with gcc '')   optimize='-O2'   case "$uname_minus_m" in   ppc*)   # on ppc\, it seems that gcc (at least gcc 3.3.2) isn't happy   # with -O2 ; so downgrade to -O1.   optimize='-O1'   ;;   ia64*)   # This architecture has had various problems with gcc's   # in the 3.2\, 3.3\, and 3.4 releases when optimized to -O2. See   # RT #37156 for a discussion of the problem.   case "`${cc​:-gcc} -v 2>&1`" in   *"version 3.2"*|*"version 3.3"*|*"version 3.4"*)   ccflags="-fno-delete-null-pointer-checks $ccflags"   ;;   esac   ;;   esac   ;; esac

---

So do linux GCC with DEBUGGING build at -O0 or -O2?

Another question\, since Perl runs on many platforms/CCs\, is there a guarantee that referencing NULL reliable stops execution/SEGVs? What about dereferencing pointer -1? What about pointers 1 through 4096\, incase pointer 0 succeeds through libc swallowing and fixing up the signals? What about a libc/CC/OS that allocates a whole valid page at addr 0 to addr page_size? I dont remember how big the interp struct is\, 2 KB??? but it would be bad if addr 1.5 KB was readable\, even though addr 0 SEGVs.

http​://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/options.htm#opt-capsz

---

-Z Command-Line Option

-Z

This option allows dereferencing of null pointers at runtime. This is the default. The value of a dereferenced null pointer is zero.

---

Perl uses -z flag to turn this off this "feature" on HP aCC.

I've thought about adding some sanity tests to XS-APItest to make sure that things that are supposed to crash\, CRASH! A nice mis-feature of the MS libc was\, that there is a place where a try/catch [everything] block was silently swallowing SEGVs from heap corruption\, and the libc function returns success (0) to perl interp. Many PP lines later\, the perl interp crashes since the same malloc block/addr was being handed out over and over by malloc. I've attached a crude patch I wrote for myself to test that various things that should crash\, crash. Perhaps it can be cleaned up by someone else and added for regular smoke testing. Some thought is required to make sure that a smoke box isn't going to quickly run out of disk space with core dumps while smoking.

Illegal and privileged instruction are CPU specific. on 386\, NULL bytes for instructions might not crash immediately (or ever) unless memory protection settings (ie\, page not marked as code) causes a SEGV https​://en.wikipedia.org/wiki/Data_Execution_Prevention . 0xFF/-1 is also valid on some CPUs. Only with PARISC and SPARC do 0xff and 0x00 both stop execution. Info below made with https://www.onlinedisassembler.com/odaweb/

386 .data​:00000000 0000 add BYTE PTR [eax]\,al .data​:00000002 0000 add BYTE PTR [eax]\,al
.data​:00000004 0000 add BYTE PTR [eax]\,al .data​:00000006 0000 add BYTE PTR [eax]\,al
.data​:00000000 ff (bad) .data​:00000001 ff (bad)
.data​:00000002 ff (bad)
.data​:00000003 ff (bad)
.data​:00000004 ff (bad) ;
.data​:00000005 ff (bad)
.data​:00000006 ff (bad)
.data​:00000007 ff .byte 0xff

ARM .data​:00000000 00000000 andeq r0\, r0\, r0
.data​:00000004 00000000 andeq r0\, r0\, r0 .data​:00000000 ffffffff; \ instruction​: 0xffffffff
.data​:00000004 ffffffff; \ instruction​: 0xffffffff ;

PARISC .data​:00000000 00000000 break 0\,0
.data​:00000004 00000000 break 0\,0 .data​:00000000 ffffffff #ffffffff
.data​:00000004 ffffffff #ffffffff

MIPS .data​:00000000 00000000 nop
.data​:00000004 00000000 nop .data​:00000000 ffffffff sd ra\,-1(ra) ; store doubleword .data​:00000004 ffffffff sd ra\,-1(ra) ; store doubleword

POWERPC .data​:00000000 00000000 .long 0x0
.data​:00000004 00000000 .long 0x0 .data​:00000000 ffffffff fnmadd. f31\,f31\,f31\,f31
.data​:00000004 ffffffff fnmadd. f31\,f31\,f31\,f31
SPARC .data​:00000000 00000000 unimp 0
.data​:00000004 00000000 unimp 0 .data​:00000000 ffffffff unknown
.data​:00000004 ffffffff unknown

-- bulk88 ~ bulk88 at hotmail.com

[p5pRT]

From @bulk88

0001-add-intentional-crashing-tests.patch

```diff From 01a65cb100c4e3e38c9b2c73183294ab92d2e67e Mon Sep 17 00:00:00 2001 From: Daniel Dragan Date: Fri, 23 Jan 2015 00:06:37 -0500 Subject: [PATCH] add intentional crashing tests --- MANIFEST | 1 + crashtest.pl | 12 +++++++ ext/XS-APItest/APItest.xs | 79 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 0 deletions(-) create mode 100644 crashtest.pl diff --git a/MANIFEST b/MANIFEST index 92a836d..8b805eb 100644 --- a/MANIFEST +++ b/MANIFEST @@ -2851,6 +2851,7 @@ cpan/Win32/t/Names.t See if Win32 extension works cpan/Win32/t/Unicode.t See if Win32 extension works cpan/Win32/Win32.pm Win32 extension Perl module cpan/Win32/Win32.xs Win32 extension external subroutines +crashtest.pl bulk88's testing of core crashing on win32 Cross/build-arm-n770-sh Cross-compilation Cross/cflags-cross-arm Cross-compilation Cross/config Cross-compilation diff --git a/crashtest.pl b/crashtest.pl new file mode 100644 index 0000000..98902d1 --- /dev/null +++ b/crashtest.pl @@ -0,0 +1,12 @@ +use Win32API::File; + +sub runtest { +my $fn = shift; +my $r = system(1, 'perl -Ilib -MXS::APItest -E"XS::APItest::'.$fn.'()"'); +my $p = wait(); +printf($fn.' $? %x CHILD_ERROR_NATIVE %x'."\n", $?, ${^CHILD_ERROR_NATIVE}); +} + +Win32API::File::SetErrorMode(Win32API::File::SEM_NOGPFAULTERRORBOX()); +runtest($_) foreach(qw(disable_interrupts illegal_instruction deref_null + deref_neg1 write_to_ro_mem div_by_0 call_c_debugger)); diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs index 075bb4d..8d79bfd 100644 --- a/ext/XS-APItest/APItest.xs +++ b/ext/XS-APItest/APItest.xs @@ -8,6 +8,28 @@ #include "XSUB.h" #include "fakesdio.h" /* Causes us to use PerlIO below */ +#ifdef WIN32 +# include "dos.h" +# ifdef _MSC_VER +# pragma intrinsic(_disable) +# else + /* even though _disable is declared as a func in dos.h, it isn't available */ +# define _disable() __asm__("cli") +# endif +# ifdef USING_MSVC6 +# pragma code_seg(".text") +# else +# pragma code_seg(push, ".text") +# endif +/* 0x0F 0x0B UD2 ins, 0xC3 retn ins, VC 64 doesnt support inline asm */ +__declspec(allocate(".text")) U8 ud2_ins[3] = { 0x0F, 0x0B, 0xC3 }; +# ifdef USING_MSVC6 +# pragma code_seg() +# else +# pragma code_seg(pop) +# endif +#endif + typedef SV *SVREF; typedef PTR_TBL_t *XS__APItest__PtrTable; @@ -3908,6 +3930,63 @@ test_newOP_CUSTOM() OUTPUT: RETVAL +#ifdef WIN32 +void +disable_interrupts() +PPCODE: + /* disabling interrupts is illegal from user mode, causes EXCEPTION_PRIV_INSTRUCTION */ + _disable(); + +void +illegal_instruction() +PREINIT: + void (*fud2)() = (void (*)()) (void*)ud2_ins; +PPCODE: + fud2(); + +void +call_c_debugger() +PPCODE: + DebugBreak(); + +#endif + +void +deref_null() +PREINIT: + int *nowhere = NULL; +PPCODE: + *nowhere = 0; + +void +deref_neg1() +PREINIT: + int *nowhere = (int*)(SSize_t)-1; +PPCODE: + *nowhere = 0; + +void +write_to_ro_mem() +PREINIT: + int *nowhere = (int*)PL_no_aelem; +PPCODE: + *nowhere = 0; + +UV +div_by_0(...) +PREINIT: + UV divisor; +CODE: + /* defeat CC optimizer */ + if(items >= 1) + divisor = SvUV(ST(0)); + else + divisor = 0; + RETVAL = 1 / divisor; +OUTPUT: + RETVAL + + MODULE = XS::APItest PACKAGE = XS::APItest::AUTOLOADtest int -- 1.7.9.msysgit.0 ```

[p5pRT]

From @tonycoz

On Thu Jul 09 07​:02​:33 2015\, davem wrote​:

On Thu\, Jul 09\, 2015 at 11​:47​:11AM +0300\, Jarkko Hietaniemi wrote​:

Given all the pitfalls with the nonnull attribute (ranging from misleading and confusing to outright dangerous) could someone remind me of the benefits?

Off-hand... Mostly it seems to just give permission for optimizers to do surprising things. Exploiting undefined behaviour by "this cannot happen so let's remove it". It certainly doesn't magically make code "NULL-resistant".

If we only need to generating our own assertions\, maybe we instead should use some attribute of our own?

I think perhaps two things are getting conflated here by us and/or gcc​:

1) telling the compiler that we *know* that function X will never be called with a null argument N (because reasons) and that therefore it's safe to optimise with that assumption - similar to the way we say that we *know* that certain die-like functions can never return;

2) telling the run-time that this function *shouldn't* be called with a null arg\, and to helpfully abort if it does (on debugging builds anyway).

I think the PERL_ARGS_ASSERT_* are doing (2)\, while the __attribute__((nonnull(2))) is for (1).

The NN info we add to embed.fnc is for (2)\, so it's probably wrong for us to generate the __attribute__ in addition to the ASSERT. If so\, it may have been wrong with the recent removal of various "if (!p) croak()" at the starts of functions. Perhaps PERL_ARGS_ASSERT_* on non-debugging fields should automatically inject lots of 'if (!p) croak("panic​: ..")' statements instead?

To me the correct solution is​:

1) for -DDEBUGGING builds\, don't use __attribute_nonnull__()\, but have the asserts\, this (hopefully) prevents the compiler optimizing the assert()s out.

2) for non-DEBUGGING builds\, use __attribute_nonnull__() for the optimization improvements\, and don't have the asserts.

Currently we have 2)\, since we always have the attributes\, and the asserts compile to nothing.

Do we want 1) ?

Ideally we wouldn't​:

  #define __attribute__nonnull(x)

in debugging builds - it may interfere with system headers.

Tony

[p5pRT]

From @ap

* Tony Cook via RT \perlbug\-followup@&#8203;perl\.org [2015-07-21 06​:45]​:

To me the correct solution is​:

1) for -DDEBUGGING builds\, don't use __attribute_nonnull__()\, but have the asserts\, this (hopefully) prevents the compiler optimizing the assert()s out.

2) for non-DEBUGGING builds\, use __attribute_nonnull__() for the optimization improvements\, and don't have the asserts.

We NEVER want those optimization (dis)improvements. Even where we do\, we do not want them from __attribute_nonnull__.

GCC does not actually check the call sites of a function annotated with __attribute_nonnull__ for whether they could pass NULL to it. It simply takes the attribute as a *promise from the programmer to the compiler* that the function will never be called with NULL arguments.

When given such a promise\, it silently elides `if (!somepointer)` checks inside the function that we explicitly wrote into it.

What’s the use of that? If we could truthfully make this promise to the compiler\, we could just manually delete all those `if (!somepointer)` checks ourselves.

In fact we should! Leaving them in the code but then obliquely eliding them by adding __attribute_nonnull__ to tell the compiler to pretend like they aren’t there is not optimisation\, it’s obfuscation.

And of course this all means that we can only make use of this attribute extremely cautiously\, after a very careful audit of the code to check that\, indeed\, there isn’t a single call site that ever COULD call this function with NULL – as opposed to the current approach of annotating anything and everything that looks vaguely eligible.

This in turns means we also canNOT annotate ANY function that is exposed API\, because we cannot audit all XS code in the world. While it would be correct to place the blame for any resulting crashes at the feet of the incorrect XS code\, we nevertheless share responsibility for said crashes.

Of course if we elide the NULL checks from a function\, we would want to add an assert against it ever being called with it. Now wouldn’t it be useful if the compiler could help us with that? Maybe someday someone will invent an attribute for that…

The upshot is that __attribute_nonnull__ is worse than useless\, at least under GCC.

Regards\, -- Aristotle Pagaltzis // \<http​://plasmasturm.org/>

[p5pRT]

From perl5-porters@perl.org

Aristotle wrote​:

The upshot is that __attribute_nonnull__ is worse than useless\, at least under GCC.

I think you are missing the fact that\, under debugging builds\, we catch all the mistakes\, and then under production builds we benefit from the optimisation.

Or am I missing something?

[p5pRT]

From @ap

* Father Chrysostomos \sprout@&#8203;cpan\.org [2015-07-21 18​:10]​:

Aristotle wrote​:

The upshot is that __attribute_nonnull__ is worse than useless\, at least under GCC.

I think you are missing the fact that\, under debugging builds\, we catch all the mistakes\, and then under production builds we benefit from the optimisation.

Or am I missing something?

What is this “optimisation” you speak of? That some checks get removed?

I could maybe understand if __attribute_nonnull__ somehow made callers use a faster calling convention or something. But nothing like that happens!

The *only* “optimisation” is some code gets omitted from the function. Do you consider #ifdef DEBUG inadequate for same purpose?

If you encounter

  if (!src) return FALSE;

and this is a purely defensive check which is unnecessary in production builds\, why does that not say

  #ifdef DEBUG   if (!src) return FALSE;   #endif

so that it’s absolutely clear just from looking at the code that this check will not happen in production builds? Is there any good reason we are obfuscating #ifdef DEBUG by calling it __attribute_nonnull__ here?

OTOH\, if that check *is* necessary in a production build\, then why on Earth are we declaring src an __attribute_nonnull__ so that the check can be omitted by the compiler and segvs can happen?

Can you think of *any* scenario in which __attribute_nonnull__ is not better written as either #ifdef DEBUG or the empty string?

Regards\, -- Aristotle Pagaltzis // \<http​://plasmasturm.org/>

[p5pRT]

From @Leont

On Tue\, Jul 21\, 2015 at 6​:07 PM\, Father Chrysostomos \sprout@&#8203;cpan\.org wrote​:

Aristotle wrote​:

The upshot is that __attribute_nonnull__ is worse than useless\, at least under GCC.

I think you are missing the fact that\, under debugging builds\, we catch all the mistakes\, and then under production builds we benefit from the optimisation.

Or am I missing something?

On production builds\, that attribute should be a noop not an optimization. If there is a check and a NULL is handled gracefully\, then clearly it shouldn't have been marked NN in the first place.

Leon

[p5pRT]

From @jhi

s there any good reason we

are obfuscating #ifdef DEBUG by calling it __attribute_nonnull__ here?

OTOH\, if that check *is* necessary in a production build\, then why on Earth are we declaring src an __attribute_nonnull__ so that the check can be omitted by the compiler and segvs can happen?

Can you think of *any* scenario in which __attribute_nonnull__ is not better written as either #ifdef DEBUG or the empty string?

To put it other way\, why would one ever want in

int foo(bar* p) {   assert(p);   ... }

the assert() to be *REMOVED* in a production build? (and only enabled in debug builds)?

The removal would mean absolute confidence that nobody never calls foo() with a NULL argument. And will never ever call.

Or to spin it in yet another absurd way​: "in production builds it is okay to dereference NULL".

(There is whole another discussion about whether and when\, if ever\, code should abort() (effectively\, exit)\, instead of returning a failure code\, especially in a library/platform-like application like Perl\, but that is a huge discussion in itself.)

[p5pRT]

From @tonycoz

On Tue\, Jul 21\, 2015 at 01​:01​:36PM +0200\, Aristotle Pagaltzis wrote​:

* Tony Cook via RT \perlbug\-followup@&#8203;perl\.org [2015-07-21 06​:45]​:

To me the correct solution is​:

1) for -DDEBUGGING builds\, don't use __attribute_nonnull__()\, but have the asserts\, this (hopefully) prevents the compiler optimizing the assert()s out.

2) for non-DEBUGGING builds\, use __attribute_nonnull__() for the optimization improvements\, and don't have the asserts.

We NEVER want those optimization (dis)improvements. Even where we do\, we do not want them from __attribute_nonnull__.

GCC does not actually check the call sites of a function annotated with __attribute_nonnull__ for whether they could pass NULL to it. It simply takes the attribute as a *promise from the programmer to the compiler* that the function will never be called with NULL arguments.

When given such a promise\, it silently elides `if (!somepointer)` checks inside the function that we explicitly wrote into it.

What’s the use of that? If we could truthfully make this promise to the compiler\, we could just manually delete all those `if (!somepointer)` checks ourselves.

In some cases the if (!somepointer) test is embedded in a macro\, and if we've told the compiler a given value is non-NULL then that test should be removed when we're creating a non-DEBUGGING build.

In a DEBUGGING build\, all those checks should occur.

Tony

[p5pRT]

From @kentfredric

On 21 July 2015 at 15​:21\, bulk88 via RT \perlbug\-followup@&#8203;perl\.org wrote​:

So do linux GCC with DEBUGGING build at -O0 or -O2?

In case its relevant\, this flag is happening at -O0 now\, apparently​:

gcc --help=optimizers -Q -O0 | grep delete   -fdelete-null-pointer-checks [enabled]

In some of the articles Riba cited\, some people observed `-fno-delete-null-pointer-checks` having no effect.

-- Kent

KENTNL - https://metacpan.org/author/KENTNL