C startup code: make crtbegin code safe for clang

Discussion:

(too old to reply)

Stefan Kempf

2016-08-01 17:28:34 UTC

The constructor and destructor tables are declared as arrays with one
non-NULL element. Walking those until a NULL element is reached looks
like out-of-bound accesses to newer compilers, and they turn the code
into infinite loops (e.g. clang 3.8), because it is undefined behavior.

Use constructor/destructor calling code that should be legal in both the
gcc and clang C dialect, to hopefully be immune from undefined behavior
optimizations in the future.

While there, clean up crtbegin.c and crtbegin.S a little and make them
more similar.

ok?

Index: lib/csu/crtbegin.c
===================================================================
RCS file: /cvs/src/lib/csu/crtbegin.c,v
retrieving revision 1.20
diff -u -p -r1.20 crtbegin.c
--- lib/csu/crtbegin.c 10 Nov 2015 04:14:03 -0000 1.20
+++ lib/csu/crtbegin.c 1 Aug 2016 16:56:31 -0000
@@ -85,36 +85,37 @@ long __guard_local __dso_hidden __attrib

static const init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static const init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");

static void __dtors(void) __used;
static void __ctors(void) __used;

static void
-__ctors()
+__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- const init_f *p;
+ int i;
+
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;

- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ while (i > 0) {
+ ctor_list[i]();
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--)
- (**p--)();
}

static void
-__dtors()
+__dtors(void)
{
- const init_f *p = __DTOR_LIST__ + 1;
+ int i;

- while (*p)
- (**p++)();
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}

void __init(void);
@@ -130,8 +131,8 @@ MD_SECT_CALL_FUNC(".init", __do_init);
MD_SECT_CALL_FUNC(".fini", __do_fini);

-void
-__do_init()
+static void
+__do_init(void)
{
static int initialized = 0;
static struct dwarf2_eh_object object;
@@ -146,14 +147,14 @@ __do_init()
__register_frame_info(__EH_FRAME_BEGIN__, &object);
if (__JCR_LIST__[0] && _Jv_RegisterClasses)
_Jv_RegisterClasses(__JCR_LIST__);
- (__ctors)();
+ __ctors();

atexit(__fini);
}
}

-void
-__do_fini()
+static void
+__do_fini(void)
{
static int finalized = 0;

@@ -162,7 +163,7 @@ __do_fini()
/*
* Call global destructors.
*/
- (__dtors)();
+ __dtors();
}
}

Index: lib/csu/crtbeginS.c
===================================================================
RCS file: /cvs/src/lib/csu/crtbeginS.c,v
retrieving revision 1.16
diff -u -p -r1.16 crtbeginS.c
--- lib/csu/crtbeginS.c 7 Apr 2015 01:27:06 -0000 1.16
+++ lib/csu/crtbeginS.c 1 Aug 2016 16:56:31 -0000
@@ -38,7 +38,6 @@
* constructors and destructors. The first element contains the
* number of pointers in each.
* The tables are also null-terminated.
-
*/
#include <stdlib.h>

@@ -96,39 +95,39 @@ asm(".hidden pthread_atfork\n .weak pthr

static init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");

static void __dtors(void) __used;
static void __ctors(void) __used;

-void
+static void
__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- init_f *p;
+ int i;

- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;
+
+ while (i > 0) {
+ ctor_list[i]();
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--) {
- (**p--)();
- }
}

static void
__dtors(void)
{
- init_f *p = __DTOR_LIST__ + 1;
+ int i;

- while (*p) {
- (**p++)();
- }
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}
+
void _init(void);
void _fini(void);
static void _do_init(void) __used;
@@ -141,7 +140,7 @@ MD_SECTION_PROLOGUE(".fini", _fini);
MD_SECT_CALL_FUNC(".init", _do_init);
MD_SECT_CALL_FUNC(".fini", _do_fini);

-void
+static void
_do_init(void)
{
static int initialized = 0;
@@ -159,7 +158,7 @@ _do_init(void)
}
}

-void
+static void
_do_fini(void)
{
static int finalized = 0;

Jeremie Courreges-Anglas

2016-08-01 18:30:33 UTC

Permalink

Post by Stefan Kempf
The constructor and destructor tables are declared as arrays with one
non-NULL element. Walking those until a NULL element is reached looks
like out-of-bound accesses to newer compilers, and they turn the code
into infinite loops (e.g. clang 3.8), because it is undefined behavior.
Use constructor/destructor calling code that should be legal in both the
gcc and clang C dialect, to hopefully be immune from undefined behavior
optimizations in the future.
While there, clean up crtbegin.c and crtbegin.S a little and make them
more similar.
ok?
Index: lib/csu/crtbegin.c
===================================================================
RCS file: /cvs/src/lib/csu/crtbegin.c,v
retrieving revision 1.20
diff -u -p -r1.20 crtbegin.c
--- lib/csu/crtbegin.c 10 Nov 2015 04:14:03 -0000 1.20
+++ lib/csu/crtbegin.c 1 Aug 2016 16:56:31 -0000
@@ -85,36 +85,37 @@ long __guard_local __dso_hidden __attrib
static const init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static const init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");
static void __dtors(void) __used;
static void __ctors(void) __used;
static void
-__ctors()
+__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- const init_f *p;
+ int i;
+
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;
- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ while (i > 0) {
+ ctor_list[i]();

The existing code tries to retrieve the number of valid ctors entries
from __CTOR_LIST__[0], only when that number is -1 it tries to find
the actual value by walking the array.

The ld(1) info page states:

The symbol `__CTOR_LIST__' marks the start of the global
constructors, and the symbol `__CTOR_END__' marks the end.
Similarly, `__DTOR_LIST__' and `__DTOR_END__' mark the start and
end of the global destructors. The first word in the list is the
number of entries, followed by the address of each constructor or
destructor, followed by a zero word. The compiler must arrange to
actually run the code. For these object file formats GNU C++
normally calls constructors from a subroutine `__main'; a call to
`__main' is automatically inserted into the startup code for
`main'. GNU C++ normally runs destructors either by using
`atexit', or directly from the function `exit'.

If that is correct your code should behave the same. But what if...?

Post by Stefan Kempf
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--)
- (**p--)();
}
static void
-__dtors()
+__dtors(void)
{
- const init_f *p = __DTOR_LIST__ + 1;
+ int i;
- while (*p)
- (**p++)();
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}
void __init(void);
@@ -130,8 +131,8 @@ MD_SECT_CALL_FUNC(".init", __do_init);
MD_SECT_CALL_FUNC(".fini", __do_fini);
-void
-__do_init()
+static void
+__do_init(void)
{
static int initialized = 0;
static struct dwarf2_eh_object object;
@@ -146,14 +147,14 @@ __do_init()
__register_frame_info(__EH_FRAME_BEGIN__, &object);
if (__JCR_LIST__[0] && _Jv_RegisterClasses)
_Jv_RegisterClasses(__JCR_LIST__);
- (__ctors)();
+ __ctors();
atexit(__fini);
}
}
-void
-__do_fini()
+static void
+__do_fini(void)
{
static int finalized = 0;
@@ -162,7 +163,7 @@ __do_fini()
/*
* Call global destructors.
*/
- (__dtors)();
+ __dtors();
}
}
Index: lib/csu/crtbeginS.c
===================================================================
RCS file: /cvs/src/lib/csu/crtbeginS.c,v
retrieving revision 1.16
diff -u -p -r1.16 crtbeginS.c
--- lib/csu/crtbeginS.c 7 Apr 2015 01:27:06 -0000 1.16
+++ lib/csu/crtbeginS.c 1 Aug 2016 16:56:31 -0000
@@ -38,7 +38,6 @@
* constructors and destructors. The first element contains the
* number of pointers in each.
* The tables are also null-terminated.
-
*/
#include <stdlib.h>
@@ -96,39 +95,39 @@ asm(".hidden pthread_atfork\n .weak pthr
static init_f __CTOR_LIST__[1]
- __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".ctors"))) = { (void *)-1 }; /* XXX */
static init_f __DTOR_LIST__[1]
- __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+ __used __attribute__((section(".dtors"))) = { (void *)-1 }; /* XXX */
+
+extern const init_f ctor_list[] asm(".ctors");
+extern const init_f dtor_list[] asm(".dtors");
static void __dtors(void) __used;
static void __ctors(void) __used;
-void
+static void
__ctors(void)
{
- unsigned long i = (unsigned long) __CTOR_LIST__[0];
- init_f *p;
+ int i;
- if (i == -1) {
- for (i = 1; __CTOR_LIST__[i] != NULL; i++)
- ;
+ for (i = 0; ctor_list[i + 1] != NULL; i++)
+ continue;
+
+ while (i > 0) {
+ ctor_list[i]();
i--;
}
- p = __CTOR_LIST__ + i;
- while (i--) {
- (**p--)();
- }
}
static void
__dtors(void)
{
- init_f *p = __DTOR_LIST__ + 1;
+ int i;
- while (*p) {
- (**p++)();
- }
+ for (i = 1; dtor_list[i] != NULL; i++)
+ dtor_list[i]();
}
+
void _init(void);
void _fini(void);
static void _do_init(void) __used;
@@ -141,7 +140,7 @@ MD_SECTION_PROLOGUE(".fini", _fini);
MD_SECT_CALL_FUNC(".init", _do_init);
MD_SECT_CALL_FUNC(".fini", _do_fini);
-void
+static void
_do_init(void)
{
static int initialized = 0;
@@ -159,7 +158,7 @@ _do_init(void)
}
}
-void
+static void
_do_fini(void)
{
static int finalized = 0;

--
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE

Mark Kettenis

2016-08-01 18:45:17 UTC

Permalink

Date: Mon, 01 Aug 2016 20:30:33 +0200

The existing code tries to retrieve the number of valid ctors entries
from __CTOR_LIST__[0], only when that number is -1 it tries to find
the actual value by walking the array.
The symbol `__CTOR_LIST__' marks the start of the global
constructors, and the symbol `__CTOR_END__' marks the end.
Similarly, `__DTOR_LIST__' and `__DTOR_END__' mark the start and
end of the global destructors. The first word in the list is the
number of entries, followed by the address of each constructor or
destructor, followed by a zero word. The compiler must arrange to
actually run the code. For these object file formats GNU C++
normally calls constructors from a subroutine `__main'; a call to
`__main' is automatically inserted into the startup code for
`main'. GNU C++ normally runs destructors either by using
`atexit', or directly from the function `exit'.
If that is correct your code should behave the same. But what if...?

Right, the code is not equivalent. We'd have to look carefully at gcc
and ld to see if that matters.

Also, aren't ctor_list and dtor_list polluting the namespace?

Philip Guenther

2016-08-02 04:24:42 UTC

Permalink

Post by Mark Kettenis

Date: Mon, 01 Aug 2016 20:30:33 +0200

So it needs to reference the pointers via an extern incomplete array?
Can we move to setting up the leading count via the linker script
magic documented in the ld info page, and then just use __CTOR_LIST__
as the extern array?

Post by Mark Kettenis

Post by Stefan Kempf
While there, clean up crtbegin.c and crtbegin.S a little and make them
more similar.

I'm fine with this being done...once we sure we actually have nailed
down what are the actual changes necessary to get the code to compile
correctly without changing behavior.

...

Post by Mark Kettenis

The existing code tries to retrieve the number of valid ctors entries
from __CTOR_LIST__[0], only when that number is -1 it tries to find
the actual value by walking the array.

Since we only have one version of ld doing linking now, can't we
switch from the -1 to the real count version?

...

Post by Mark Kettenis
Also, aren't ctor_list and dtor_list polluting the namespace?

Yep.

Philip

Stefan Kempf

2016-08-03 18:20:21 UTC

Permalink

Post by Philip Guenther

Post by Mark Kettenis

Date: Mon, 01 Aug 2016 20:30:33 +0200

Right, we need the extern trick.
I'll take a look into doing it with counts and __CTOR_LIST__ and
__CTOR_END__. We want __CTOR_LIST__ and __CTOR_END__ to be .hidden
symbols, right?

Post by Philip Guenther

Post by Mark Kettenis

Post by Stefan Kempf
While there, clean up crtbegin.c and crtbegin.S a little and make them
more similar.

I'm fine with this being done...once we sure we actually have nailed
down what are the actual changes necessary to get the code to compile
correctly without changing behavior.
...

Post by Mark Kettenis

The existing code tries to retrieve the number of valid ctors entries
from __CTOR_LIST__[0], only when that number is -1 it tries to find
the actual value by walking the array.

Since we only have one version of ld doing linking now, can't we
switch from the -1 to the real count version?
...

Post by Mark Kettenis
Also, aren't ctor_list and dtor_list polluting the namespace?

Yep.
Philip

Joerg Sonnenberger

2016-08-04 17:05:25 UTC

Permalink

I don't think your code is safe for the more aggressive behavior esp. of
newer GCC. Given how much fun I had recently on this, consider just
using the NetBSD version of this code.

Joerg