Re: [PATCH] reduce size of struct __libc by using narrower integers

[Rich Felker <dalias@libc.org>]

On Tue, Mar 06, 2018 at 05:55:02PM +0100, Denys Vlasenko wrote:
> can_do_threads, threaded and secure are boolean flags, can use bytes
> instead of full word ints.
> 
> tls_align and page_size are surely smaller than 4GB, no need to use
> potentially 64-bit size_t.
> 
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> CC: musl <musl@lists.openwall.com>
> ---
>  ldso/dynlink.c      |  3 ++-
>  src/internal/libc.h | 11 ++++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index 9bf6924b..67d36395 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -124,8 +124,9 @@ static jmp_buf *rtld_fail;
>  static pthread_rwlock_t lock;
>  static struct debug debug;
>  static struct tls_module *tls_tail;
> -static size_t tls_cnt, tls_offset, tls_align = MIN_TLS_ALIGN;
> +static size_t tls_cnt, tls_offset;
>  static size_t static_tls_cnt;
> +static unsigned tls_align = MIN_TLS_ALIGN;
>  static pthread_mutex_t init_fini_lock = { ._m_type = PTHREAD_MUTEX_RECURSIVE };
>  static struct fdpic_loadmap *app_loadmap;
>  static struct fdpic_dummy_loadmap app_dummy_loadmap;
> diff --git a/src/internal/libc.h b/src/internal/libc.h
> index 5e145183..3cd44e90 100644
> --- a/src/internal/libc.h
> +++ b/src/internal/libc.h
> @@ -18,14 +18,15 @@ struct tls_module {
>  };
>  
>  struct __libc {
> -	int can_do_threads;
> -	int threaded;
> -	int secure;
> +	char can_do_threads;
> +	char threaded;
> +	char secure;
>  	volatile int threads_minus_1;
>  	size_t *auxv;
>  	struct tls_module *tls_head;
> -	size_t tls_size, tls_align, tls_cnt;
> -	size_t page_size;
> +	size_t tls_size, tls_cnt;
> +	unsigned tls_align;
> +	unsigned page_size;
>  	struct __locale_struct global_locale;
>  };

This might be okay but needs some care, as it introduces some subtle
changes wrt types. For instance, PAGE_SIZE is #defined (internally for
musl) using libc.page_size when it's not constant for the arch, and
unsigned int will promote & compare differently against signed longs
than size_t would. However, I think you've actually shed light on a
bigger bug here: PAGE_SIZE, when it is defined in the public headers,
has the wrong type (int rather than unsigned int or unsigned long). It
needs to be fixed to use U or UL suffix. I'll do this along with the
PAGESIZE/PAGE_SIZE swap logic (pending namespace issue) right away.

The int->char changes are only valid if we only store positive values,
since char has arch-specific signedness, but I think it's true that we
only store 0 or 1.

For tls_align it's true that >4GB would be ridiculous and breaking,
but there are no checks to see that a loaded module does not have
ridiculous alignment requirements except for failure of the allocation
requests. If we change to a type that can't represent all possible
inputs, there needs to be a check to reject large alignments anywhere
they could arise. I think that means __init_tls.c and dynlink.c's
load_library().

Rich