Re: [PATCH 21/28] elf: Add endianness markup to ld.so.cache

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 01/10/2020 13:33, Florian Weimer via Libc-alpha wrote:
> Use a reserved byte in the new format cache header to indicate whether
> the file is in little endian or big endian format.  Eventually, this
> information could be used to provide a unified cache for qemu-user
> and similiar scenarios.

Some comments below.

> ---
>  elf/cache.c                | 11 ++++++++++
>  elf/dl-cache.c             | 20 +++++++++++++++++-
>  sysdeps/generic/dl-cache.h | 43 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/cache.c b/elf/cache.c
> index 1eb1455883..e0aa616352 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -152,6 +152,14 @@ print_entry (const char *lib, int flag, unsigned int osversion,
>    printf (") => %s\n", key);
>  }
>  
> +/* Print an error and exit if the new-file cache is internally
> +   inconsistent.  */
> +static void
> +check_new_cache (struct cache_file_new *cache)
> +{
> +  if (! cache_file_new_matches_endian (cache))
> +    error (EXIT_FAILURE, 0, _("Cache file has wrong endianness.\n"));
> +}
>  
>  /* Print the whole cache file, if a file contains the new cache format
>     hidden in the old one, print the contents of the new format.  */

Ok.

> @@ -193,6 +201,7 @@ print_cache (const char *cache_name)
>  	  || memcmp (cache_new->version, CACHE_VERSION,
>  		      sizeof CACHE_VERSION - 1))
>  	error (EXIT_FAILURE, 0, _("File is not a cache file.\n"));
> +      check_new_cache (cache_new);
>        format = 1;
>        /* This is where the strings start.  */
>        cache_data = (const char *) cache_new;

Ok.

> @@ -222,6 +231,7 @@ print_cache (const char *cache_name)
>  	      && memcmp (cache_new->version, CACHE_VERSION,
>  			 sizeof CACHE_VERSION - 1) == 0)
>  	    {
> +	      check_new_cache (cache_new);
>  	      cache_data = (const char *) cache_new;
>  	      format = 1;
>  	    }

Ok.

> @@ -361,6 +371,7 @@ save_cache (const char *cache_name)
>  
>        file_entries_new->nlibs = cache_entry_count;
>        file_entries_new->len_strings = total_strlen;
> +      file_entries_new->flags = cache_file_new_flags_endian;
>      }
>  
>    /* Pad for alignment of cache_file_new.  */

Ok.

> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 93d185e788..3aa8ed6c13 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -210,6 +210,11 @@ _dl_load_cache_lookup (const char *name)
>  	  && ((cachesize - sizeof *cache_new) / sizeof (struct file_entry_new)
>  	      >= ((struct cache_file_new *) file)->nlibs))
>  	{
> +	  if (! cache_file_new_matches_endian (file))
> +	    {
> +	      __munmap (file, cachesize);
> +	      file = (void *) -1;
> +	    }
>  	  cache_new = file;
>  	  cache = file;
>  	}

Ok.

> @@ -231,7 +236,20 @@ _dl_load_cache_lookup (const char *name)
>  	  if (cachesize < (offset + sizeof (struct cache_file_new))
>  	      || memcmp (cache_new->magic, CACHEMAGIC_VERSION_NEW,
>  			 sizeof CACHEMAGIC_VERSION_NEW - 1) != 0)
> -	    cache_new = (void *) -1;
> +	      cache_new = (void *) -1;
> +	  else
> +	    {
> +	      if (! cache_file_new_matches_endian (cache_new))
> +		{
> +		  /* The old-format part of the cache is bogus as well
> +		     if the endianness does not match.  (But it is
> +		     unclear how the new header can be located if the
> +		     endianess does not match.)  */
> +		  cache = (void *) -1;
> +		  cache_new = (void *) -1;
> +		  __munmap (file, cachesize);
> +		}
> +	    }
>  	}
>        else
>  	{

Ok.

> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index 6b310e9e15..1b04211f6b 100644
> --- a/sysdeps/generic/dl-cache.h
> +++ b/sysdeps/generic/dl-cache.h
> @@ -16,6 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#ifndef _DL_CACHE_H
> +#define _DL_CACHE_H
> +
> +#include <endian.h>
> +#include <stdbool.h>
>  #include <stdint.h>
>  
>  #ifndef _DL_CACHE_DEFAULT_ID

Ok.

> @@ -83,21 +88,57 @@ struct file_entry_new
>    uint64_t hwcap;		/* Hwcap entry.	 */
>  };
>  
> +/* See flags member of struct cache_file_new below.  */
> +enum
> +  {
> +   cache_file_new_flags_endian = (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +				  ? 2 : 3)

Why not enumerate the possible values as described below:

enum cache_file_new_flags
  {
    cache_file_flag_invalid = 1,
    cache_file_flag_endianess = 2
  };
enum
  {
    cache_file_new_flags_endian = (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
				   ? cache_file_flag_endianess : 0)
				  | cache_file_flag_invalid
  };

I think this is slight less confusing than using direct values
and more explicit on how the default flags value should be.

> +  };
> +
>  struct cache_file_new
>  {
>    char magic[sizeof CACHEMAGIC_NEW - 1];
>    char version[sizeof CACHE_VERSION - 1];
>    uint32_t nlibs;		/* Number of entries.  */
>    uint32_t len_strings;		/* Size of string table. */
> -  uint32_t unused[5];		/* Leave space for future extensions
> +
> +  /* flags & 3 is used to indicate the endianness of the cache.
> +     0: no endianness information available
> +        (An old ldconfig version without endianness support wrote the file.)
> +     1: cache is invalid
> +     2: little endian
> +     3: big endian
> +
> +     The remaining bits are unused and should be generated as zero and
> +     ignored by readers.  */
> +  uint8_t flags;
> +
> +  uint8_t padding_unsed[3];	/* Not used, for future extensions.  */
> +
> +  uint32_t unused[4];		/* Leave space for future extensions
>  				   and align to 8 byte boundary.  */
>    struct file_entry_new libs[0]; /* Entries describing libraries.  */
>    /* After this the string table of size len_strings is found.	*/
>  };
>  
> +/* Returns false if *CACHE has the wrong endianness for this
> +   architecture, and true if the endianness matches (or is
> +   unknown).  */
> +static inline bool
> +cache_file_new_matches_endian (const struct cache_file_new *cache)
> +{
> +  /* A zero value for cache->flags means that no endianness
> +     information is available.  */
> +  return cache->flags == 0
> +    || (cache->flags & 3) == cache_file_new_flags_endian;
> +}
> +
> +

Using the suggestion above:

static inline bool
cache_file_new_matches_endian (const struct cache_file_new *cache)
{
  return cache->flags == 0
	 || (cache->flags & (cache_file_flag_endianess | cache_file_flag_invalid))
            == cache_file_flag_endianess;
}

>  /* Used to align cache_file_new.  */
>  #define ALIGN_CACHE(addr)				\
>  (((addr) + __alignof__ (struct cache_file_new) -1)	\
>   & (~(__alignof__ (struct cache_file_new) - 1)))
>  
>  extern int _dl_cache_libcmp (const char *p1, const char *p2) attribute_hidden;
> +
> +#endif /* _DL_CACHE_H */
> 

Ok.