On 10/6/21 11:20 PM, YU Jincheng wrote:
Does this path have any problem?

From: YU Jincheng <[email protected]>

- clang 9+ will load the const pointer first before the const

   pointer assignment trick and thus cause null pointer defer.

- This patch creates a shadow variable to prevent the above from

   happening for clang 9+.

- Also, this patch applies `BB_GLOBAL_CONST` to all variables

   using the same trick, allowing archs or toolchains having the

   same issue to bypass this trick correctly.

This patch fixes https://bugs.busybox.net/show_bug.cgi?id=14231

and https://bugs.busybox.net/show_bug.cgi?id=14236

Co-authored-by: canyie <[email protected]>

Signed-off-by: YU Jincheng <[email protected]>


We have been carrying a fix for similar problem here
https://github.com/kraj/meta-clang/blob/master/recipes-core/busybox/busybox/0001-Turn-ptr_to_globals-and-bb_errno-to-be-non-const.patch

does this patch fixes your issue as well ?

---

coreutils/test.c  |  7 ++-----

include/libbb.h   | 34 +++++++++++++++++++++++++++++-----

libbb/appletlib.c |  3 +--

libbb/lineedit.c  |  7 ++-----

shell/ash.c       | 23 +++--------------------

5 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/coreutils/test.c b/coreutils/test.c

index 7c6574334..3a91de407 100644

--- a/coreutils/test.c

+++ b/coreutils/test.c

@@ -435,7 +435,7 @@ struct test_statics {

};

  /* See test_ptr_hack.c */

-extern struct test_statics *const test_ptr_to_statics;

+extern struct test_statics *BB_GLOBAL_CONST test_ptr_to_statics;

  #define S (*test_ptr_to_statics)

#define args            (S.args         )

@@ -445,10 +445,7 @@ extern struct test_statics *const test_ptr_to_statics;

#define bash_test2      (S.bash_test2   )

#define leaving         (S.leaving      )

-#define INIT_S() do { \

-             (*(struct test_statics**)not_const_pp(&test_ptr_to_statics)) = xzalloc(sizeof(S)); \

-             barrier(); \

-} while (0)

+#define INIT_S() ASSIGN_CONST_PTR(test_ptr_to_statics, xzalloc(sizeof(S)))

#define DEINIT_S() do { \

               free(group_array); \

               free(test_ptr_to_statics); \

diff --git a/include/libbb.h b/include/libbb.h

index dfcaa05ec..cdc8bd271 100644

--- a/include/libbb.h

+++ b/include/libbb.h

@@ -365,10 +365,24 @@ struct BUG_off_t_size_is_misdetected {

#endif

#endif

+/* We use a trick to have more optimized code (fewer pointer reloads

+ * and reduce binary size by a few kilobytes) like:

+ *  ash.c:   extern struct globals *const ash_ptr_to_globals;

+ *  ash_ptr_hack.c: struct globals *ash_ptr_to_globals;

+ * This way, compiler in ash.c knows the pointer can not change.

+ *

+ * However, this may break on weird arches or toolchains. In this case,

+ * set "-DBB_GLOBAL_CONST=''" in CONFIG_EXTRA_CFLAGS to disable

+ * this optimization.

+ */

+#ifndef BB_GLOBAL_CONST

+# define BB_GLOBAL_CONST const

+#endif

+

#if defined(errno)

/* If errno is a define, assume it's "define errno (*__errno_location())"

   * and we will cache it's result in this variable */

-extern int *const bb_errno;

+extern int *BB_GLOBAL_CONST bb_errno;

#undef errno

#define errno (*bb_errno)

#define bb_cached_errno_ptr 1

@@ -2284,16 +2298,26 @@ static ALWAYS_INLINE void* not_const_pp(const void *p)

               );

               return pp;

}

+#ifndef ASSIGN_CONST_PTR

+#define ASSIGN_CONST_PTR(p, v) \

+_Pragma("clang diagnostic push"); \

+_Pragma("clang diagnostic ignored \"-Wshadow\""); \

+             __typeof__(p) p = ({*(void**)not_const_pp(&p) = (void*)(v);});

+_Pragma("clang diagnostic pop")

+#endif

#else

static ALWAYS_INLINE void* not_const_pp(const void *p) { return (void*)p; }

+#ifndef ASSIGN_CONST_PTR

+#define ASSIGN_CONST_PTR(p, v) do { \

+             (*(void**)not_const_pp(&p)) = (void*)(v); \

+             barrier(); \

+} while (0)

+#endif

#endif

  /* At least gcc 3.4.6 on mipsel system needs optimization barrier */

#define barrier() __asm__ __volatile__("":::"memory")

-#define SET_PTR_TO_GLOBALS(x) do { \

-             (*(struct globals**)not_const_pp(&ptr_to_globals)) = (void*)(x); \

-             barrier(); \

-} while (0)

+#define SET_PTR_TO_GLOBALS(x) ASSIGN_CONST_PTR(ptr_to_globals, x)

  #define FREE_PTR_TO_GLOBALS() do { \

               if (ENABLE_FEATURE_CLEAN_UP) { \

diff --git a/libbb/appletlib.c b/libbb/appletlib.c

index 14be33603..8b4c0f0e7 100644

--- a/libbb/appletlib.c

+++ b/libbb/appletlib.c

@@ -247,8 +247,7 @@ void lbb_prepare(const char *applet

                              IF_FEATURE_INDIVIDUAL(, char **argv))

{

#ifdef bb_cached_errno_ptr

-             (*(int **)not_const_pp(&bb_errno)) = get_perrno();

-             barrier();

+             ASSIGN_CONST_PTR(bb_errno, get_perrno());

#endif

               applet_name = applet;

diff --git a/libbb/lineedit.c b/libbb/lineedit.c

index a7a3ee103..73194282c 100644

--- a/libbb/lineedit.c

+++ b/libbb/lineedit.c

@@ -192,7 +192,7 @@ struct lineedit_statics {

};

  /* See lineedit_ptr_hack.c */

-extern struct lineedit_statics *const lineedit_ptr_to_statics;

+extern struct lineedit_statics *BB_GLOBAL_CONST lineedit_ptr_to_statics;

  #define S (*lineedit_ptr_to_statics)

#define state            (S.state           )

@@ -213,10 +213,7 @@ extern struct lineedit_statics *const lineedit_ptr_to_statics;

#define newdelflag       (S.newdelflag      )

#define delbuf           (S.delbuf          )

-#define INIT_S() do { \

-             (*(struct lineedit_statics**)not_const_pp(&lineedit_ptr_to_statics)) = xzalloc(sizeof(S)); \

-             barrier(); \

-} while (0)

+#define INIT_S() ASSIGN_CONST_PTR(lineedit_ptr_to_statics, xzalloc(sizeof(S)))

  static void deinit_S(void)

{

diff --git a/shell/ash.c b/shell/ash.c

index 4bc4f55d0..c8a20ab94 100644

--- a/shell/ash.c

+++ b/shell/ash.c

@@ -303,20 +303,6 @@ typedef long arith_t;

# error "Do not even bother, ash will not run on NOMMU machine"

#endif

-/* We use a trick to have more optimized code (fewer pointer reloads):

- *  ash.c:   extern struct globals *const ash_ptr_to_globals;

- *  ash_ptr_hack.c: struct globals *ash_ptr_to_globals;

- * This way, compiler in ash.c knows the pointer can not change.

- *

- * However, this may break on weird arches or toolchains. In this case,

- * set "-DBB_GLOBAL_CONST=''" in CONFIG_EXTRA_CFLAGS to disable

- * this optimization.

- */

-#ifndef BB_GLOBAL_CONST

-# define BB_GLOBAL_CONST const

-#endif

-

-

/* ============ Hash table sizes. Configurable. */

  #define VTABSIZE 39

@@ -518,8 +504,7 @@ extern struct globals_misc *BB_GLOBAL_CONST ash_ptr_to_globals_misc;

#define random_gen  (G_misc.random_gen )

#define backgndpid  (G_misc.backgndpid )

#define INIT_G_misc() do { \

-             (*(struct globals_misc**)not_const_pp(&ash_ptr_to_globals_misc)) = xzalloc(sizeof(G_misc)); \

-             barrier(); \

+             ASSIGN_CONST_PTR(ash_ptr_to_globals_misc, xzalloc(sizeof(G_misc))); \

               savestatus = -1; \

               curdir = nullstr; \

               physdir = nullstr; \

@@ -1597,8 +1582,7 @@ extern struct globals_memstack *BB_GLOBAL_CONST ash_ptr_to_globals_memstack;

#define g_stacknleft (G_memstack.g_stacknleft)

#define stackbase    (G_memstack.stackbase   )

#define INIT_G_memstack() do { \

-             (*(struct globals_memstack**)not_const_pp(&ash_ptr_to_globals_memstack)) = xzalloc(sizeof(G_memstack)); \

-             barrier(); \

+             ASSIGN_CONST_PTR(ash_ptr_to_globals_memstack, xzalloc(sizeof(G_memstack))); \

               g_stackp = &stackbase; \

               g_stacknxt = stackbase.space; \

               g_stacknleft = MINSIZE; \

@@ -2229,8 +2213,7 @@ extern struct globals_var *BB_GLOBAL_CONST ash_ptr_to_globals_var;

#endif

#define INIT_G_var() do { \

               unsigned i; \

-             (*(struct globals_var**)not_const_pp(&ash_ptr_to_globals_var)) = xzalloc(sizeof(G_var)); \

-             barrier(); \

+             ASSIGN_CONST_PTR(ash_ptr_to_globals_var, xzalloc(sizeof(G_var))); \

               for (i = 0; i < ARRAY_SIZE(varinit_data); i++) { \

                              varinit[i].flags    = varinit_data[i].flags; \

                             varinit[i].var_text = varinit_data[i].var_text; \

--

2.25.1


_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox

_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox

Reply via email to