| Message ID | 87v8a83f9t.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ldconfig: Fixes for skipping temporary files. | expand |
On 11/11/2023 07:44, Florian Weimer wrote: > Arguments to a memchr call were swapped, causing incorrect skipping > of files. [Apologies if this reply turns up twice, my MUA glitched out in an "interesting" way] Would like to +1 the memchr fix, the arg swap broke our pressure-vessel test runs quite severely - we ended up with badly broken container library contents.
* Vivek Dasmohapatra: > On 11/11/2023 07:44, Florian Weimer wrote: >> Arguments to a memchr call were swapped, causing incorrect skipping >> of files. > > [Apologies if this reply turns up twice, my MUA glitched out in an > "interesting" way] > > Would like to +1 the memchr fix, the arg swap broke our pressure-vessel > test runs quite severely - we ended up with badly broken container > library contents. Thanks, I've interpreted this as a review and pushed the patch. We can tweak it subsequently if necessary. Florian
On 20/11/2023 10:19, Florian Weimer wrote: > > Thanks, I've interpreted this as a review and pushed the patch. We can > tweak it subsequently if necessary. Great: For the record, to be explicit: rest of the patch lgtm too.
On 11/11/23 04:44, Florian Weimer wrote: > Arguments to a memchr call were swapped, causing incorrect skipping > of files. > > Files related to dpkg have different names: they actually end in > .dpkg-new and .dpkg-tmp, not .tmp as I mistakenly assumed. > > Fixes commit 2aa0974d2573441bffd59 ("elf: ldconfig should skip > temporary files created by package managers"). > > Tested on i686-linux-gnu and x86_64-linux-gnu. The new version fixes > the memchr swapped arguments bug, too. > > Thanks, > Florian > > --- > elf/ldconfig.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index 02387a169c..bccd386761 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -661,6 +661,17 @@ struct dlib_entry > struct dlib_entry *next; > }; > > +/* Return true if the N bytes at NAME end with with the characters in > + the string SUFFIX. (NAME[N + 1] does not have to be a null byte.) > + Expected to be called with a string literal for SUFFIX. */ > +static inline bool > +endswithn (const char *name, size_t n, const char *suffix) > +{ > + return (n >= strlen (suffix) > + && memcmp (name + n - strlen (suffix), suffix, > + strlen (suffix)) == 0); > +} > + > /* Skip some temporary DSO files. These files may be partially written > and lead to ldconfig crashes when examined. */ > static bool > @@ -670,8 +681,7 @@ skip_dso_based_on_name (const char *name, size_t len) > names like these are never really DSOs we want to look at. */ > if (len >= sizeof (".#prelink#") - 1) > { > - if (strcmp (name + len - sizeof (".#prelink#") + 1, > - ".#prelink#") == 0) > + if (endswithn (name, len, ".#prelink#")) > return true; > if (len >= sizeof (".#prelink#.XXXXXX") - 1 > && memcmp (name + len - sizeof (".#prelink#.XXXXXX") > @@ -679,10 +689,11 @@ skip_dso_based_on_name (const char *name, size_t len) > return true; > } > /* Skip temporary files created by RPM. */ > - if (memchr (name, len, ';') != NULL) > + if (memchr (name, ';', len) != NULL) Should we check whether -Wconversion would be useful to use on glibc build? It would catch such issue. Also, I noted that all elf subdir, including ldconfig, is being built without fortify: Makeconfig: 970 # We must filter out elf because the early bootstrap of the dynamic loader 971 # cannot be fortified. Likewise we exclude dlfcn because it is entangled 972 # with the loader. We must filter out csu because early startup, like the 973 # loader, cannot be fortified. Lastly debug is the fortification routines 974 # themselves and they cannot be fortified. 975 do-fortify = $(filter-out elf dlfcn csu debug,$(subdir)) 976 ifeq ($(do-fortify),$(subdir)) 977 +cflags += $(+fortify-source) 978 else 979 +cflags += $(no-fortify-source) 980 endif I think we should enable for the installed programs at least (ldconfig, sln, sprof). > return true; > /* Skip temporary files created by dpkg. */ > - if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) > + if (endswithn (name, len, ".dpkg-new") > + || endswithn (name, len, ".dpkg-tmp")) > return true; > return false; > } > > base-commit: d1dcb565a1fb5829f9476a1438c30eccc4027d04 >
diff --git a/elf/ldconfig.c b/elf/ldconfig.c index 02387a169c..bccd386761 100644 --- a/elf/ldconfig.c +++ b/elf/ldconfig.c @@ -661,6 +661,17 @@ struct dlib_entry struct dlib_entry *next; }; +/* Return true if the N bytes at NAME end with with the characters in + the string SUFFIX. (NAME[N + 1] does not have to be a null byte.) + Expected to be called with a string literal for SUFFIX. */ +static inline bool +endswithn (const char *name, size_t n, const char *suffix) +{ + return (n >= strlen (suffix) + && memcmp (name + n - strlen (suffix), suffix, + strlen (suffix)) == 0); +} + /* Skip some temporary DSO files. These files may be partially written and lead to ldconfig crashes when examined. */ static bool @@ -670,8 +681,7 @@ skip_dso_based_on_name (const char *name, size_t len) names like these are never really DSOs we want to look at. */ if (len >= sizeof (".#prelink#") - 1) { - if (strcmp (name + len - sizeof (".#prelink#") + 1, - ".#prelink#") == 0) + if (endswithn (name, len, ".#prelink#")) return true; if (len >= sizeof (".#prelink#.XXXXXX") - 1 && memcmp (name + len - sizeof (".#prelink#.XXXXXX") @@ -679,10 +689,11 @@ skip_dso_based_on_name (const char *name, size_t len) return true; } /* Skip temporary files created by RPM. */ - if (memchr (name, len, ';') != NULL) + if (memchr (name, ';', len) != NULL) return true; /* Skip temporary files created by dpkg. */ - if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) + if (endswithn (name, len, ".dpkg-new") + || endswithn (name, len, ".dpkg-tmp")) return true; return false; }
Arguments to a memchr call were swapped, causing incorrect skipping of files. Files related to dpkg have different names: they actually end in .dpkg-new and .dpkg-tmp, not .tmp as I mistakenly assumed. Fixes commit 2aa0974d2573441bffd59 ("elf: ldconfig should skip temporary files created by package managers"). Tested on i686-linux-gnu and x86_64-linux-gnu. The new version fixes the memchr swapped arguments bug, too. Thanks, Florian --- elf/ldconfig.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) base-commit: d1dcb565a1fb5829f9476a1438c30eccc4027d04