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​:

<Nicholas> I'd suggest mailing p5-sec, because that creates an RT ticket.
<Nicholas> if "we" think that it's not an "OMG panic" job, it's trivial
to move the ticket to the public queue
<Nicholas> the GCC NN stuff is really bloody annoying because you can't
add a "not null" annotation to variables or structure elements
<Nicholas> *just* function pointers
<Nicholas> but our plan was to have assertions everywhere which check
the not-NULL-ness
<Nicholas> (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

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​:

<Nicholas> I'd suggest mailing p5-sec, because that creates an RT ticket.
<Nicholas> if "we" think that it's not an "OMG panic" job, it's trivial
to move the ticket to the public queue
<Nicholas> the GCC NN stuff is really bloody annoying because you can't
add a "not null" annotation to variables or structure elements
<Nicholas> *just* function pointers
<Nicholas> but our plan was to have assertions everywhere which check
the not-NULL-ness
<Nicholas> (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 7918f24
Author​: Nicholas Clark <nick@​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 cf684db
Author​: Nicholas Clark <nick@​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

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

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@​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​:

<Nicholas> I'd suggest mailing p5-sec, because that creates an RT ticket.
<Nicholas> if "we" think that it's not an "OMG panic" job, it's trivial
to move the ticket to the public queue
<Nicholas> the GCC NN stuff is really bloody annoying because you can't
add a "not null" annotation to variables or structure elements
<Nicholas> *just* function pointers
<Nicholas> but our plan was to have assertions everywhere which check
the not-NULL-ness
<Nicholas> (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 7918f24
Author​: Nicholas Clark <nick@​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 cf684db
Author​: Nicholas Clark <nick@​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

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.

From @Leont

On Thu, Jul 9, 2015 at 4​:01 PM, Dave Mitchell <davem@​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

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!

From @rjbs

* Peter Rabbitson <rabbit@​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

From @rjbs

This ticket has been moved to the perl5 queue.

--
rjbs

From [Unknown Contact. See original ticket]

This ticket has been moved to the perl5 queue.

--
rjbs

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; <UNDEFINED> instruction​: 0xffffffff
.data​:00000004 ffffffff; <UNDEFINED> 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

From @bulk88

From 01a65cb100c4e3e38c9b2c73183294ab92d2e67e Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
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

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

From @ap

* Tony Cook via RT <perlbug-followup@​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/>

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?

From @ap

* Father Chrysostomos <sprout@​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/>

From @Leont

On Tue, Jul 21, 2015 at 6​:07 PM, Father Chrysostomos <sprout@​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

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.)

From @tonycoz

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

* Tony Cook via RT <perlbug-followup@​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

From @kentfredric

On 21 July 2015 at 15​:21, bulk88 via RT <perlbug-followup@​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