diff mbox series

[04/11] elf: Add endianness markup to ld.so.cache

Message ID f8cac05ce355cb762f7fb00f1177d0c3d538decb.1604946656.git.fweimer@redhat.com
State Committed
Headers show
Series glibc-hwcaps support | expand

Commit Message

Florian Weimer Nov. 9, 2020, 6:40 p.m. UTC
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.
---
 elf/cache.c                | 11 +++++++
 elf/dl-cache.c             | 20 +++++++++++-
 sysdeps/generic/dl-cache.h | 62 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 91 insertions(+), 2 deletions(-)

Comments

Adhemerval Zanella Nov. 27, 2020, 1:56 p.m. UTC | #1
On 09/11/2020 15:40, 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.

LGTM, with some comments below.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/cache.c                | 11 +++++++
>  elf/dl-cache.c             | 20 +++++++++++-
>  sysdeps/generic/dl-cache.h | 62 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 91 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/cache.c b/elf/cache.c
> index c241c17ef9..ffecbe6d82 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_current;
>      }
>  
>    /* Pad for alignment of cache_file_new.  */

Ok.

> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 45894ecd2f..02c46ffb0c 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -242,6 +242,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.

> @@ -263,7 +268,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;

I think there is no need to set cache_new to -1 here, the function
will return anyway with the 'cache == (void *) -1' check below.

> +		  __munmap (file, cachesize);
> +		}
> +	    }
>  	}
>        else
>  	{
> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
> index 4ddd96b005..46026e0988 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.

> @@ -92,21 +97,76 @@ struct file_entry_new
>    uint64_t hwcap;		/* Hwcap entry.	 */
>  };
>  
> +/* See flags member of struct cache_file_new below.  */
> +enum
> +  {
> +    /* No endianness information available.  An old ldconfig version
> +       without endianness support wrote the file.  */
> +    cache_file_new_flags_endian_unset = 0,
> +
> +    /* Cache is invalid and should be ignored.  */
> +    cache_file_new_flags_endian_invalid = 1,
> +
> +    /* Cache format is little endian.  */
> +    cache_file_new_flags_endian_little = 2,
> +
> +    /* Cache format is big endian.  */
> +    cache_file_new_flags_endian_big = 3,
> +
> +    /* Bit mask to extract the cache_file_new_flags_endian_*
> +       values.  */
> +    cache_file_new_flags_endian_mask = 3,
> +
> +    /* Expected value of the endian bits in the flags member for the
> +       current architecture.  */
> +    cache_file_new_flags_endian_current
> +      = (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +	 ? cache_file_new_flags_endian_little
> +	 : cache_file_new_flags_endian_big),
> +  };
> +

Ok.

>  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 & cache_file_new_flags_endian_mask is one of the values
> +     cache_file_new_flags_endian_unset, cache_file_new_flags_endian_invalid,
> +     cache_file_new_flags_endian_little, cache_file_new_flags_endian_big.
> +
> +     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.	*/
>  };
>  

Maybe add a _Static_assert here to check the expected 
struct cache_file_new size?

> +/* 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

Maybe check against cache_file_new_flags_endian_unset ?

> +    || ((cache->flags & cache_file_new_flags_endian_big)
> +	== cache_file_new_flags_endian_current);
> +}
> +
> +
>  /* 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.
Florian Weimer Nov. 27, 2020, 7:49 p.m. UTC | #2
* Adhemerval Zanella via Libc-alpha:

>> @@ -263,7 +268,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;
>
> I think there is no need to set cache_new to -1 here, the function
> will return anyway with the 'cache == (void *) -1' check below.

Sure, but I want to make absolutely clear that cache_new is not usable
due to the incorrect endianness.

I'd like to leave it in.

>>  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 & cache_file_new_flags_endian_mask is one of the values
>> +     cache_file_new_flags_endian_unset, cache_file_new_flags_endian_invalid,
>> +     cache_file_new_flags_endian_little, cache_file_new_flags_endian_big.
>> +
>> +     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.	*/
>>  };
>>  
>
> Maybe add a _Static_assert here to check the expected 
> struct cache_file_new size?

I've added:

_Static_assert (sizeof (struct cache_file_new) == 48,
		"size of struct cache_file_new");

>> +/* 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
>
> Maybe check against cache_file_new_flags_endian_unset ?

I intended cache_file_new_flags_endian_unset to document the (reserved)
value within the masked lower two bits, not the whole flags field, so I
think 0 is correct here.

Thanks,
Florian
Adhemerval Zanella Nov. 30, 2020, 7 p.m. UTC | #3
On 27/11/2020 16:49, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>>> @@ -263,7 +268,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;
>>
>> I think there is no need to set cache_new to -1 here, the function
>> will return anyway with the 'cache == (void *) -1' check below.
> 
> Sure, but I want to make absolutely clear that cache_new is not usable
> due to the incorrect endianness.
> 
> I'd like to leave it in.

Ack.

> 
>>>  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 & cache_file_new_flags_endian_mask is one of the values
>>> +     cache_file_new_flags_endian_unset, cache_file_new_flags_endian_invalid,
>>> +     cache_file_new_flags_endian_little, cache_file_new_flags_endian_big.
>>> +
>>> +     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.	*/
>>>  };
>>>  
>>
>> Maybe add a _Static_assert here to check the expected 
>> struct cache_file_new size?
> 
> I've added:
> 
> _Static_assert (sizeof (struct cache_file_new) == 48,
> 		"size of struct cache_file_new");

LGTM.

> 
>>> +/* 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
>>
>> Maybe check against cache_file_new_flags_endian_unset ?
> 
> I intended cache_file_new_flags_endian_unset to document the (reserved)
> value within the masked lower two bits, not the whole flags field, so I
> think 0 is correct here.

Ack.

> 
> Thanks,
> Florian
>
diff mbox series

Patch

diff --git a/elf/cache.c b/elf/cache.c
index c241c17ef9..ffecbe6d82 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.  */
@@ -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;
@@ -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;
 	    }
@@ -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_current;
     }
 
   /* Pad for alignment of cache_file_new.  */
diff --git a/elf/dl-cache.c b/elf/dl-cache.c
index 45894ecd2f..02c46ffb0c 100644
--- a/elf/dl-cache.c
+++ b/elf/dl-cache.c
@@ -242,6 +242,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;
 	}
@@ -263,7 +268,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
 	{
diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
index 4ddd96b005..46026e0988 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
@@ -92,21 +97,76 @@  struct file_entry_new
   uint64_t hwcap;		/* Hwcap entry.	 */
 };
 
+/* See flags member of struct cache_file_new below.  */
+enum
+  {
+    /* No endianness information available.  An old ldconfig version
+       without endianness support wrote the file.  */
+    cache_file_new_flags_endian_unset = 0,
+
+    /* Cache is invalid and should be ignored.  */
+    cache_file_new_flags_endian_invalid = 1,
+
+    /* Cache format is little endian.  */
+    cache_file_new_flags_endian_little = 2,
+
+    /* Cache format is big endian.  */
+    cache_file_new_flags_endian_big = 3,
+
+    /* Bit mask to extract the cache_file_new_flags_endian_*
+       values.  */
+    cache_file_new_flags_endian_mask = 3,
+
+    /* Expected value of the endian bits in the flags member for the
+       current architecture.  */
+    cache_file_new_flags_endian_current
+      = (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	 ? cache_file_new_flags_endian_little
+	 : cache_file_new_flags_endian_big),
+  };
+
 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 & cache_file_new_flags_endian_mask is one of the values
+     cache_file_new_flags_endian_unset, cache_file_new_flags_endian_invalid,
+     cache_file_new_flags_endian_little, cache_file_new_flags_endian_big.
+
+     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 & cache_file_new_flags_endian_big)
+	== cache_file_new_flags_endian_current);
+}
+
+
 /* 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 */