| Message ID | 87y1fxxyrl.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | elf: ldconfig should skip temporary files created by package managers | expand |
On 20/10/23 09:29, Florian Weimer wrote: > This avoids crashes due to partially written files, after a package > update is interrupted. It seems kinda unreliable a tool create temporary files on the libdir, although we alrady handle for prelinkk so it makes sense to extend to other program as well. Maybe another option would to proper parse the file header and only handle ELF files, so ldconfig is not bounded to name patterns. > > Tested on x86_64-linux-gnu. > > --- > NEWS | 4 +++- > elf/ldconfig.c | 39 +++++++++++++++++++++++++++------------ > 2 files changed, 30 insertions(+), 13 deletions(-) > > diff --git a/NEWS b/NEWS > index cc4b81f0ac..9432564444 100644 > --- a/NEWS > +++ b/NEWS > @@ -40,7 +40,9 @@ Major new features: > > Deprecated and removed features, and other changes affecting compatibility: > > - [Add deprecations, removals and changes affecting compatibility here] > +* The ldconfig program now skips file names containing ';' or ending in > + ".tmp", to avoid examining temporary files created by the RPM and dpkg > + package managers. > > Changes to build and runtime requirements: > > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index d26eef1fb4..02387a169c 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -661,6 +661,31 @@ struct dlib_entry > struct dlib_entry *next; > }; > > +/* Skip some temporary DSO files. These files may be partially written > + and lead to ldconfig crashes when examined. */ > +static bool > +skip_dso_based_on_name (const char *name, size_t len) > +{ > + /* Skip temporary files created by the prelink program. Files with > + 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) > + return true; > + if (len >= sizeof (".#prelink#.XXXXXX") - 1 > + && memcmp (name + len - sizeof (".#prelink#.XXXXXX") > + + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) > + return true; > + } > + /* Skip temporary files created by RPM. */ > + if (memchr (name, len, ';') != NULL) > + return true; > + /* Skip temporary files created by dpkg. */ > + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) Wouldn't this potentially read out the bounds for sizes [5, 8)? Maybe just use strcmp? > + return true; > + return false; > +} > > static void > search_dir (const struct dir_entry *entry) > @@ -711,18 +736,8 @@ search_dir (const struct dir_entry *entry) > continue; > > size_t len = strlen (direntry->d_name); > - /* Skip temporary files created by the prelink program. Files with > - names like these are never really DSOs we want to look at. */ > - if (len >= sizeof (".#prelink#") - 1) > - { > - if (strcmp (direntry->d_name + len - sizeof (".#prelink#") + 1, > - ".#prelink#") == 0) > - continue; > - if (len >= sizeof (".#prelink#.XXXXXX") - 1 > - && memcmp (direntry->d_name + len - sizeof (".#prelink#.XXXXXX") > - + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) > - continue; > - } > + if (skip_dso_based_on_name (direntry->d_name, len)) > + continue; > if (asprintf (&file_name, "%s/%s", entry->path, direntry->d_name) < 0) > error (EXIT_FAILURE, errno, _("Could not form library path")); > if (opt_chroot != NULL) > > base-commit: f5677d9cebb12edcd9301dbb5cf40f82618b46af >
* Adhemerval Zanella Netto: > On 20/10/23 09:29, Florian Weimer wrote: >> This avoids crashes due to partially written files, after a package >> update is interrupted. > > It seems kinda unreliable a tool create temporary files on the libdir, > although we alrady handle for prelinkk so it makes sense to extend to > other program as well. Maybe another option would to proper parse the > file header and only handle ELF files, so ldconfig is not bounded to > name patterns. The files look like ELF files, but a truncated, so that we get SIGBUS because the mapping we access is only halfway there. >> + /* Skip temporary files created by dpkg. */ >> + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) > > Wouldn't this potentially read out the bounds for sizes [5, 8)? Maybe > just use strcmp? I don't see how. ABCDE ^ name + len ^ name + len - 4 .tmp Looks okay to me? Thanks, Florian
On 20/10/23 11:41, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 20/10/23 09:29, Florian Weimer wrote: >>> This avoids crashes due to partially written files, after a package >>> update is interrupted. >> >> It seems kinda unreliable a tool create temporary files on the libdir, >> although we alrady handle for prelinkk so it makes sense to extend to >> other program as well. Maybe another option would to proper parse the >> file header and only handle ELF files, so ldconfig is not bounded to >> name patterns. > > The files look like ELF files, but a truncated, so that we get SIGBUS > because the mapping we access is only halfway there. Right, filtering by the name indeed seems less troublesome. > >>> + /* Skip temporary files created by dpkg. */ >>> + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) >> >> Wouldn't this potentially read out the bounds for sizes [5, 8)? Maybe >> just use strcmp? > > I don't see how. > > ABCDE > ^ name + len > ^ name + len - 4 > .tmp > > Looks okay to me? Yeah, you are right. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Hi! [ Hope the reply works, had to construct it from the web archive. ] On Fri Oct 20 12:29:50 GMT 2023 Florian Weimer wrote: > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index d26eef1fb4..02387a169c 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -661,6 +661,31 @@ struct dlib_entry > struct dlib_entry *next; > }; > > +/* Skip some temporary DSO files. These files may be partially written > + and lead to ldconfig crashes when examined. */ > +static bool > +skip_dso_based_on_name (const char *name, size_t len) > +{ > + /* Skip temporary files created by the prelink program. Files with > + 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) > + return true; > + if (len >= sizeof (".#prelink#.XXXXXX") - 1 > + && memcmp (name + len - sizeof (".#prelink#.XXXXXX") > + + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) > + return true; > + } > + /* Skip temporary files created by RPM. */ > + if (memchr (name, len, ';') != NULL) > + return true; > + /* Skip temporary files created by dpkg. */ > + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) > + return true; If this refers to files dpkg creates during an unpack, then for regular files, it might end up creating ones ending with «.dpkg-new» and «.dpkg-tmp» (for conffiles there are other extensions used, but those would not seem relevant here). (And, hmm, I should probably try to document that somewhere in the dpkg manual pages or docs.) > + return false; > +} > > static void > search_dir (const struct dir_entry *entry) Thanks, Guillem
On Fri, Oct 20, 2023 at 9:30 AM Florian Weimer <fweimer@redhat.com> wrote: > This avoids crashes due to partially written files, after a package > update is interrupted. > Do you know if rpm at least as been fixed not to do that ? >
* Guillem Jover: >> + /* Skip temporary files created by dpkg. */ >> + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) >> + return true; > > If this refers to files dpkg creates during an unpack, then for > regular files, it might end up creating ones ending with «.dpkg-new» > and «.dpkg-tmp» (for conffiles there are other extensions used, but > those would not seem relevant here). Ugh, I must have been momentarily confused when I made this change. I did look at strace out, but apparently promptly forget what I saw (although in my test, only .dpkg-new showed up). I'll sent a follow-up fix. Thanks, Florian
* Cristian Rodríguez: > On Fri, Oct 20, 2023 at 9:30 AM Florian Weimer <fweimer@redhat.com> wrote: > >> This avoids crashes due to partially written files, after a package >> update is interrupted. > > Do you know if rpm at least as been fixed not to do that ? I think DNF/RPM nowadays intercept SIGINT during file writing, but signals are enabled when scriptlet execution starts, so you probably still get a corrupted system when you hit ^C. I don't think there are plans to use O_TMPFILE to prevent partial files from showing up. So if there is a system crash, partial files could still be left behind even if RPM is not interrupted so easily anymore. Thanks, Florian
diff --git a/NEWS b/NEWS index cc4b81f0ac..9432564444 100644 --- a/NEWS +++ b/NEWS @@ -40,7 +40,9 @@ Major new features: Deprecated and removed features, and other changes affecting compatibility: - [Add deprecations, removals and changes affecting compatibility here] +* The ldconfig program now skips file names containing ';' or ending in + ".tmp", to avoid examining temporary files created by the RPM and dpkg + package managers. Changes to build and runtime requirements: diff --git a/elf/ldconfig.c b/elf/ldconfig.c index d26eef1fb4..02387a169c 100644 --- a/elf/ldconfig.c +++ b/elf/ldconfig.c @@ -661,6 +661,31 @@ struct dlib_entry struct dlib_entry *next; }; +/* Skip some temporary DSO files. These files may be partially written + and lead to ldconfig crashes when examined. */ +static bool +skip_dso_based_on_name (const char *name, size_t len) +{ + /* Skip temporary files created by the prelink program. Files with + 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) + return true; + if (len >= sizeof (".#prelink#.XXXXXX") - 1 + && memcmp (name + len - sizeof (".#prelink#.XXXXXX") + + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) + return true; + } + /* Skip temporary files created by RPM. */ + if (memchr (name, len, ';') != NULL) + return true; + /* Skip temporary files created by dpkg. */ + if (len > 4 && memcmp (name + len - 4, ".tmp", 4) == 0) + return true; + return false; +} static void search_dir (const struct dir_entry *entry) @@ -711,18 +736,8 @@ search_dir (const struct dir_entry *entry) continue; size_t len = strlen (direntry->d_name); - /* Skip temporary files created by the prelink program. Files with - names like these are never really DSOs we want to look at. */ - if (len >= sizeof (".#prelink#") - 1) - { - if (strcmp (direntry->d_name + len - sizeof (".#prelink#") + 1, - ".#prelink#") == 0) - continue; - if (len >= sizeof (".#prelink#.XXXXXX") - 1 - && memcmp (direntry->d_name + len - sizeof (".#prelink#.XXXXXX") - + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) - continue; - } + if (skip_dso_based_on_name (direntry->d_name, len)) + continue; if (asprintf (&file_name, "%s/%s", entry->path, direntry->d_name) < 0) error (EXIT_FAILURE, errno, _("Could not form library path")); if (opt_chroot != NULL)