[PATCH] Fix const pointer assignment trick on clang 9+
Khem Raj
raj.khem at gmail.com
Thu Oct 7 16:44:49 UTC 2021
On 10/6/21 11:20 PM, YU Jincheng wrote:
> Does this path have any problem?
>
> From: YU Jincheng <shana at zju.edu.cn>
>
> - 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 <31466456+canyie at users.noreply.github.com>
>
> Signed-off-by: YU Jincheng <shana at zju.edu.cn>
>
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
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
>
More information about the busybox
mailing list