[04/11] elf: Add endianness markup to ld.so.cache
Commit Message
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
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.
* 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
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
>
@@ -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. */
@@ -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
{
@@ -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 */