[PATCH v2] ldconfig: Fixes for skipping temporary files.

[PATCH v2] ldconfig: Fixes for skipping temporary files.

[Florian Weimer]

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)
     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

Re: [PATCH v2] ldconfig: Fixes for skipping temporary files.

[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.

Re: [PATCH v2] ldconfig: Fixes for skipping temporary files.

[Florian Weimer]

* 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

Re: [PATCH v2] ldconfig: Fixes for skipping temporary files.

[Vivek Dasmohapatra]

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.

Re: [PATCH v2] ldconfig: Fixes for skipping temporary files.

[Adhemerval Zanella Netto]

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
>