| Message ID | 20180919014833.GA18964@localhost |
|---|---|
| State | New |
| Headers | show |
| Series | [BZ,#23668] Default to the new format for ld.so.cache | expand |
Josh Triplett <josh@joshtriplett.org> writes: > glibc has supported this format for 18+ years. > > Reorder conditionals to look for the new format first. > > [BZ #23668] > * elf/ldconfig.c: Default to the new format for ld.so.cache. glibc has > supported this format for 18+ years. > * elf/dl-cache.c (_dl_load_cache_lookup): Reorder conditionals to look > for the new format first. Josh, do you know if any distribution has been using this format by default? Thanks, Florian
On Wed, Sep 19, 2018 at 09:35:57PM +0200, Florian Weimer wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > > glibc has supported this format for 18+ years. > > > > Reorder conditionals to look for the new format first. > > > > [BZ #23668] > > * elf/ldconfig.c: Default to the new format for ld.so.cache. glibc has > > supported this format for 18+ years. > > * elf/dl-cache.c (_dl_load_cache_lookup): Reorder conditionals to look > > for the new format first. > > Josh, do you know if any distribution has been using this format by > default? Clear is using it by default, and I'm currently working on getting Debian doing so.
* Josh Triplett: > diff --git a/elf/dl-cache.c b/elf/dl-cache.c > index 6ee5153ff9..75bd9d9536 100644 > --- a/elf/dl-cache.c > +++ b/elf/dl-cache.c > @@ -203,7 +203,14 @@ _dl_load_cache_lookup (const char *name) > - the old format with the new format in it > - only the new format > The following checks if the cache contains any of these formats. */ > - if (file != MAP_FAILED && cachesize > sizeof *cache > + if (file != MAP_FAILED && cachesize > sizeof *cache_new > + && memcmp (file, CACHEMAGIC_VERSION_NEW, > + sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) > + { > + cache_new = file; > + cache = file; > + } > + else if (file != MAP_FAILED && cachesize > sizeof *cache > && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0) > { > size_t offset; > @@ -220,13 +227,6 @@ _dl_load_cache_lookup (const char *name) > sizeof CACHEMAGIC_VERSION_NEW - 1) != 0) > cache_new = (void *) -1; > } > - else if (file != MAP_FAILED && cachesize > sizeof *cache_new > - && memcmp (file, CACHEMAGIC_VERSION_NEW, > - sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) > - { > - cache_new = file; > - cache = file; > - } > else > { > if (file != MAP_FAILED) I do not think this is needed. It's just a minor performance optimization, right? There's a missing consistency check on the new-format path (both with and without your patch), but we can add that separately. I think we can remove support for the old cache format completely. > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index fbdd814edf..1ce4a29566 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -95,7 +95,7 @@ int opt_verbose; > > /* Format to support. */ > /* 0: only libc5/glibc2; 1: both; 2: only glibc 2.2. */ > -int opt_format = 1; > +int opt_format = 2; > > /* Build cache. */ > static int opt_build_cache = 1; > @@ -148,7 +148,7 @@ static const struct argp_option options[] = > { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file"), 0}, > { NULL, 'n', NULL, 0, N_("Only process directories specified on the command line. Don't build cache."), 0}, > { NULL, 'l', NULL, 0, N_("Manually link individual libraries."), 0}, > - { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new, old or compat (default)"), 0}, > + { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new (default), old, or compat"), 0}, > { "ignore-aux-cache", 'i', NULL, 0, N_("Ignore auxiliary cache file"), 0}, > { NULL, 0, NULL, 0, NULL, 0 } > }; But this patch a good start. Do you need help installing it? Thanks, Florian
On Fri, May 08, 2020 at 08:58:40PM +0200, Florian Weimer wrote: > * Josh Triplett: > > diff --git a/elf/dl-cache.c b/elf/dl-cache.c > > index 6ee5153ff9..75bd9d9536 100644 > > --- a/elf/dl-cache.c > > +++ b/elf/dl-cache.c > > @@ -203,7 +203,14 @@ _dl_load_cache_lookup (const char *name) > > - the old format with the new format in it > > - only the new format > > The following checks if the cache contains any of these formats. */ > > - if (file != MAP_FAILED && cachesize > sizeof *cache > > + if (file != MAP_FAILED && cachesize > sizeof *cache_new > > + && memcmp (file, CACHEMAGIC_VERSION_NEW, > > + sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) > > + { > > + cache_new = file; > > + cache = file; > > + } > > + else if (file != MAP_FAILED && cachesize > sizeof *cache > > && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0) > > { > > size_t offset; > > @@ -220,13 +227,6 @@ _dl_load_cache_lookup (const char *name) > > sizeof CACHEMAGIC_VERSION_NEW - 1) != 0) > > cache_new = (void *) -1; > > } > > - else if (file != MAP_FAILED && cachesize > sizeof *cache_new > > - && memcmp (file, CACHEMAGIC_VERSION_NEW, > > - sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) > > - { > > - cache_new = file; > > - cache = file; > > - } > > else > > { > > if (file != MAP_FAILED) > > I do not think this is needed. It's just a minor performance > optimization, right? A minor performance optimization at the start of every single dynamically linked program run; I think it's worth doing. > I think we can remove support for the old cache format completely. I didn't want to make that call, given glibc's strong compatibility guarantees. It seems reasonable to remove, but perhaps we should deprecate it first, in case someone's system still has an old-format cache and they don't realize that? > > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > > index fbdd814edf..1ce4a29566 100644 > > --- a/elf/ldconfig.c > > +++ b/elf/ldconfig.c > > @@ -95,7 +95,7 @@ int opt_verbose; > > > > /* Format to support. */ > > /* 0: only libc5/glibc2; 1: both; 2: only glibc 2.2. */ > > -int opt_format = 1; > > +int opt_format = 2; > > > > /* Build cache. */ > > static int opt_build_cache = 1; > > @@ -148,7 +148,7 @@ static const struct argp_option options[] = > > { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file"), 0}, > > { NULL, 'n', NULL, 0, N_("Only process directories specified on the command line. Don't build cache."), 0}, > > { NULL, 'l', NULL, 0, N_("Manually link individual libraries."), 0}, > > - { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new, old or compat (default)"), 0}, > > + { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new (default), old, or compat"), 0}, > > { "ignore-aux-cache", 'i', NULL, 0, N_("Ignore auxiliary cache file"), 0}, > > { NULL, 0, NULL, 0, NULL, 0 } > > }; > > But this patch a good start. Do you need help installing it? I'd appreciate it if someone with commit access would commit it, yes. I don't have such access. - Josh Triplett
* Josh Triplett: > On Fri, May 08, 2020 at 08:58:40PM +0200, Florian Weimer wrote: >> * Josh Triplett: >> > diff --git a/elf/dl-cache.c b/elf/dl-cache.c >> > index 6ee5153ff9..75bd9d9536 100644 >> > --- a/elf/dl-cache.c >> > +++ b/elf/dl-cache.c >> > @@ -203,7 +203,14 @@ _dl_load_cache_lookup (const char *name) >> > - the old format with the new format in it >> > - only the new format >> > The following checks if the cache contains any of these formats. */ >> > - if (file != MAP_FAILED && cachesize > sizeof *cache >> > + if (file != MAP_FAILED && cachesize > sizeof *cache_new >> > + && memcmp (file, CACHEMAGIC_VERSION_NEW, >> > + sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) >> > + { >> > + cache_new = file; >> > + cache = file; >> > + } >> > + else if (file != MAP_FAILED && cachesize > sizeof *cache >> > && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0) >> > { >> > size_t offset; >> > @@ -220,13 +227,6 @@ _dl_load_cache_lookup (const char *name) >> > sizeof CACHEMAGIC_VERSION_NEW - 1) != 0) >> > cache_new = (void *) -1; >> > } >> > - else if (file != MAP_FAILED && cachesize > sizeof *cache_new >> > - && memcmp (file, CACHEMAGIC_VERSION_NEW, >> > - sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) >> > - { >> > - cache_new = file; >> > - cache = file; >> > - } >> > else >> > { >> > if (file != MAP_FAILED) >> >> I do not think this is needed. It's just a minor performance >> optimization, right? > > A minor performance optimization at the start of every single > dynamically linked program run; I think it's worth doing. Fair enough. I'm going push it without this change and rework the optimization so not to regress bug 18093. I don't know if you have copyright paperwork with the FSF; these additional changes would likely exceed the limit of what we can accept without such paperwork. >> I think we can remove support for the old cache format completely. > > I didn't want to make that call, given glibc's strong compatibility > guarantees. It seems reasonable to remove, but perhaps we should > deprecate it first, in case someone's system still has an old-format > cache and they don't realize that? Make sense. Note that this is not about the ability of skipping over an old-format header, we have to keep this for fifteen or so more years. Thanks, Florian
diff --git a/NEWS b/NEWS index f76ada94d3..95ca9f5888 100644 --- a/NEWS +++ b/NEWS @@ -37,6 +37,9 @@ Deprecated and removed features, and other changes affecting compatibility: used by the Linux kernel. This affects the size and layout of those structures. +* ldconfig now defaults to the new format for ld.so.cache. glibc has supported + this format for 18+ years. + Changes to build and runtime requirements: [Add changes to build and runtime requirements here] diff --git a/elf/dl-cache.c b/elf/dl-cache.c index 6ee5153ff9..75bd9d9536 100644 --- a/elf/dl-cache.c +++ b/elf/dl-cache.c @@ -203,7 +203,14 @@ _dl_load_cache_lookup (const char *name) - the old format with the new format in it - only the new format The following checks if the cache contains any of these formats. */ - if (file != MAP_FAILED && cachesize > sizeof *cache + if (file != MAP_FAILED && cachesize > sizeof *cache_new + && memcmp (file, CACHEMAGIC_VERSION_NEW, + sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) + { + cache_new = file; + cache = file; + } + else if (file != MAP_FAILED && cachesize > sizeof *cache && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0) { size_t offset; @@ -220,13 +227,6 @@ _dl_load_cache_lookup (const char *name) sizeof CACHEMAGIC_VERSION_NEW - 1) != 0) cache_new = (void *) -1; } - else if (file != MAP_FAILED && cachesize > sizeof *cache_new - && memcmp (file, CACHEMAGIC_VERSION_NEW, - sizeof CACHEMAGIC_VERSION_NEW - 1) == 0) - { - cache_new = file; - cache = file; - } else { if (file != MAP_FAILED) diff --git a/elf/ldconfig.c b/elf/ldconfig.c index fbdd814edf..1ce4a29566 100644 --- a/elf/ldconfig.c +++ b/elf/ldconfig.c @@ -95,7 +95,7 @@ int opt_verbose; /* Format to support. */ /* 0: only libc5/glibc2; 1: both; 2: only glibc 2.2. */ -int opt_format = 1; +int opt_format = 2; /* Build cache. */ static int opt_build_cache = 1; @@ -148,7 +148,7 @@ static const struct argp_option options[] = { NULL, 'f', N_("CONF"), 0, N_("Use CONF as configuration file"), 0}, { NULL, 'n', NULL, 0, N_("Only process directories specified on the command line. Don't build cache."), 0}, { NULL, 'l', NULL, 0, N_("Manually link individual libraries."), 0}, - { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new, old or compat (default)"), 0}, + { "format", 'c', N_("FORMAT"), 0, N_("Format to use: new (default), old, or compat"), 0}, { "ignore-aux-cache", 'i', NULL, 0, N_("Ignore auxiliary cache file"), 0}, { NULL, 0, NULL, 0, NULL, 0 } };