[v2] ldconfig: Fixes for skipping temporary files.
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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
Comments
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
>
@@ -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;
}