Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
On Sun, Oct 02, 2022 at 04:00:58PM +0100, Colin Watson wrote: > Control: tag -1 fixed-upstream > Success! > https://gitlab.com/cjwatson/man-db/-/compare/5d2594d0a0...866c3571d3 awesome! On Sun, Oct 02, 2022 at 05:56:19PM +0100, Colin Watson wrote: > I thought I'd set SOURCE_DATE_EPOCH, but I'd failed to pass it through > sudo. After fixing that, I indeed get cmp-identical tarballs. very nice! much cheers! -- cheers, Holger ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄ Plastic bottles: made to last forever, designed to throw away. signature.asc Description: PGP signature
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
On Sun, Oct 02, 2022 at 05:50:07PM +0200, Johannes Schauer Marin Rodrigues wrote: > Quoting Colin Watson (2022-10-02 17:00:58) > > As well as more localized testing, I built a .deb with this and used > > josch's instructions from the start of this bug to build mmdebstrap > > tarballs via disorderfs, using > > "--hook-dir=/usr/share/mmdebstrap/hooks/file-mirror-automount > > --include=./man-db_2.10.3~20221002-1_amd64.deb" to inject the new .deb. > > The two resulting tarballs had somewhat differing file lists (timestamps > > etc.), but all the actual files in the tarballs were bitwise-identical. > > Did you maybe forget the "export SOURCE_DATE_EPOCH=XXX" step? Just replace XXX > with the output of `date +%s` but make sure that both mmdebstrap invocations > see the same value for SOURCE_DATE_EPOCH and then there should be zero > differences and a "cmp" should be sufficient to make sure that it works. I thought I'd set SOURCE_DATE_EPOCH, but I'd failed to pass it through sudo. After fixing that, I indeed get cmp-identical tarballs. -- Colin Watson (he/him) [cjwat...@debian.org]
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Quoting Colin Watson (2022-10-02 17:00:58) > Success! > > https://gitlab.com/cjwatson/man-db/-/compare/5d2594d0a0...866c3571d3 Thank you!! :D > > As well as more localized testing, I built a .deb with this and used > josch's instructions from the start of this bug to build mmdebstrap > tarballs via disorderfs, using > "--hook-dir=/usr/share/mmdebstrap/hooks/file-mirror-automount > --include=./man-db_2.10.3~20221002-1_amd64.deb" to inject the new .deb. > The two resulting tarballs had somewhat differing file lists (timestamps > etc.), but all the actual files in the tarballs were bitwise-identical. Did you maybe forget the "export SOURCE_DATE_EPOCH=XXX" step? Just replace XXX with the output of `date +%s` but make sure that both mmdebstrap invocations see the same value for SOURCE_DATE_EPOCH and then there should be zero differences and a "cmp" should be sufficient to make sure that it works. Thanks! cheers, josch signature.asc Description: signature
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Control: tag -1 fixed-upstream Success! https://gitlab.com/cjwatson/man-db/-/compare/5d2594d0a0...866c3571d3 As well as more localized testing, I built a .deb with this and used josch's instructions from the start of this bug to build mmdebstrap tarballs via disorderfs, using "--hook-dir=/usr/share/mmdebstrap/hooks/file-mirror-automount --include=./man-db_2.10.3~20221002-1_amd64.deb" to inject the new .deb. The two resulting tarballs had somewhat differing file lists (timestamps etc.), but all the actual files in the tarballs were bitwise-identical. Feel free to do any other testing you think might be useful. There's a bootstrapped source tarball attached as an artifact to the "build-distcheck" CI job in GitLab that you can easily use to build a snapshot .deb if you need one. -- Colin Watson (he/him) [cjwat...@debian.org]
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Hi Colin, On Sun, Sep 25, 2022 at 11:18:19PM +0100, Colin Watson wrote: > This weekend's work has been: > https://gitlab.com/cjwatson/man-db/-/compare/bb0f7086ba...5d2594d0a0 wow, impressive! (and thank you for taking care of man-db for so many years now! :) [...] > I'll need a bit more concentrated hacking time here, but I'll continue > to work on these; this has been a great opportunity to clean up some > truly unpleasant bits of code. Once I have the accessdb diff down to > zero, we'll see whether there's any further instability in the on-disk > GDBM representation, and also whether there are any other issues that > don't show up in the set of pages I have installed. sounds great! also thank you for keeping us updated here, i'm looking forward to hear more good news eventually! :) -- cheers, Holger ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄ I'm looking forward to Corona being a beer again and Donald a duck. signature.asc Description: PGP signature
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
This weekend's work has been: https://gitlab.com/cjwatson/man-db/-/compare/bb0f7086ba...5d2594d0a0 A lot of this was code rearrangement that I needed to do before I could make progress on the real issues, but if you look at the NEWS.md diff you'll see a number of changes that relate to this bug. With all of that, there are 33 lines of diff of accessdb output remaining on my system against the result of josch's patch, which come down to two issues: * unstable choice of whatis target for pages with many entries in NAME, some but not all of which are represented as symlinks in the filesystem to a file name that is not itself in NAME (there are some examples of this in libbsd-dev and libmd-dev) * some difficulty deciding exactly what to do with cross-section links in some cases (inetd.conf(5) → inetd(8)) I'll need a bit more concentrated hacking time here, but I'll continue to work on these; this has been a great opportunity to clean up some truly unpleasant bits of code. Once I have the accessdb diff down to zero, we'll see whether there's any further instability in the on-disk GDBM representation, and also whether there are any other issues that don't show up in the set of pages I have installed. -- Colin Watson (he/him) [cjwat...@debian.org]
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Hi Colin, On Thu, Sep 22, 2022 at 08:53:07PM +0100, Colin Watson wrote: > Yeah, this has taken me a bit longer than expected, but I have in fact > been making some progress. josch's patch has been very useful in that > it provides an easy way to see differences between unsorted and sorted > traversal, and I've taken my goal as being to drive those differences to > zero. The only bit I've committed so far has been: > > > https://gitlab.com/cjwatson/man-db/-/commit/bb0f7086ba4ce4503761737bf612088c03b6c495 cool, thanks for the update and all your man-db work! > I'll update this bug as I make further progress. great, thanks again! -- cheers, Holger ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄ Imagine god created trillions of galaxies but freaks out because some dude kisses another. signature.asc Description: PGP signature
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Control: tag -1 - patch On Thu, Sep 22, 2022 at 03:48:30PM +, Holger Levsen wrote: > Colin, what's the status of this bug? You said you were working on improving > josch' patch in May 2022...?! :) Yeah, this has taken me a bit longer than expected, but I have in fact been making some progress. josch's patch has been very useful in that it provides an easy way to see differences between unsorted and sorted traversal, and I've taken my goal as being to drive those differences to zero. The only bit I've committed so far has been: https://gitlab.com/cjwatson/man-db/-/commit/bb0f7086ba4ce4503761737bf612088c03b6c495 I also have a few hundred lines of somewhat untidy patch that I'll commit in a few pieces as soon as I'm certain of it; this is all essentially about stabilizing the decisions about which database entries win compared to which other entries, so that the end result doesn't change depending on the scan order. With that, I'm down to on the order of 150 lines of diff of accessdb output against the result of josch's patch, and I think there are only about one or two problems left. A lot of the remaining difficulties are due to somewhat impenetrable old code which appeared to be trying to micro-optimize memory usage in a way that I don't think makes sense nowadays, so I may take a bit of a digression into reorganizing some of this. I'll update this bug as I make further progress. > Also, the bug is currently tagged 'patch', I guess it's appropriate to remove > that tag? Done. -- Colin Watson (he/him) [cjwat...@debian.org]
Bug#1010957: status update? Re: Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
hi! Colin, what's the status of this bug? You said you were working on improving josch' patch in May 2022...?! :) Also, the bug is currently tagged 'patch', I guess it's appropriate to remove that tag? josch: btw you said you you submitted other patches missing freeing of memory, have you updated those other patches? -- cheers, Holger ⢀⣴⠾⠻⢶⣦⠀ ⣾⠁⢠⠒⠀⣿⡁ holger@(debian|reproducible-builds|layer-acht).org ⢿⡄⠘⠷⠚⠋⠀ OpenPGP: B8BF54137B09D35CF026FE9D 091AB856069AAA1C ⠈⠳⣄ We live in a world where teenagers get more and more desperate trying to convince adults to behave like grown ups. signature.asc Description: PGP signature
Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Hi Colin,
Quoting Colin Watson (2022-05-19 01:36:37)
> On Sun, May 15, 2022 at 12:20:57AM +0200, Johannes Schauer Marin Rodrigues
> wrote:
> > I now have a patch (attached).
>
> Thanks for your patch!
>
> I found a few problems with it. Don't worry about sending a new patch
> to address these; I have fixes in my local tree for most things I've
> commented on here and am working on the rest. I'm just letting you know
> where things stand.
thank you!! :)
> First, we should add the appropriate Gnulib modules for upstream portability.
> Then:
>
> > @@ -356,8 +356,8 @@ static void add_dir_entries (MYDBM_FILE
> >* or . files (such as current, parent dir).
> >*/
> >
> > - dir = opendir (infile);
> > - if (!dir) {
> > + n = scandir(infile, , NULL, alphasort);
>
> IMO we might as well move the filtering currently being done in the
> while loop below to a scandir filter function.
>
> I would prefer not to use alphasort here, because it's locale-dependent
> (not that it will matter very much in practice with the sorts of file
> names that typically appear in manual page directories, but I can
> imagine edge cases). A variant that's deliberately locale-independent
> is only a few lines of code.
That makes sense. Thank you.
> > @@ -367,13 +367,13 @@ static void add_dir_entries (MYDBM_FILE
> >
> > /* strlen(newdir->d_name) could be replaced by newdir->d_reclen */
> >
> > - while ((newdir = readdir (dir)) != NULL) {
> > - if (*newdir->d_name == '.' &&
> > - strlen (newdir->d_name) < (size_t) 3)
> > + while (n--) {
>
> I guess this might have been borrowed from the example in the scandir(3)
> manual page, because it goes in reverse order; I don't think there's a
> good reason to do that.
In fact, my patch is borrowed from existing patches that the reproducible
builds team submitted to packages and...
> > + free(namelist);
>
> This leaks memory. We need to free all the elements of this list as
> well.
...all these patches are missing a free() of the individual list members, so
they all leak memory. Thanks a lot for spotting this -- I guess now I can
prepare a few more patches for other packages. This would not've happened if I
indeed had read the scandir(3) manual page more carefully, which does free()
correctly. XD
> > order_files (infile, );
>
> This means that the conversion to scandir in this function is in
> practice going to be ineffective, because we immediately turn around and
> re-sort the list by the physical locations of the first blocks of the
> corresponding files. Won't this have just as much of an effect on
> reproducibility in principle, even if it doesn't happen to affect
> mmdebstrap in your tests, presumably due to something like disk order
> typically being similar between runs if your disk isn't too full?
This is interesting. I've tested my patch by executing man-db on a filesystem
mounted with disorderfs --shuffle-dirents=yes and observed that using scandir()
was somehow necessary in both locations.
> I'm experimenting with simply removing the order_files call here, on the
> basis that other performance improvements to mandb(8) have made it less
> critical. It does seem to slow things down slightly even on an SSD, though
> that may be measurement error; I still need to compare timings on a
> rotational drive (but I will).
Thanks!
> More interestingly, it changes accessdb(8) output in ways that aren't just
> obvious consequences of sorting (multi keys change due to that, but as far as
> I can tell that's fine). So far I've spotted the following issues:
>
> * A number of new entries are introduced for some reason (and the
>question is why they were missing beforehand).
>
> * The targets of WHATIS_MAN references change (admittedly in cases
>where there are multiple possibilities, but we should be picking a
>deterministic one rather than just the first).
>
> * Symlinks flip between ULT_MAN and SO_MAN depending on whether the
>symlink happens to sort before its target.
>
> * The database's idea of whether pages require processing via tbl(1)
>(and probably other preprocessors) changes in some cases. (Fixed in
>https://gitlab.com/cjwatson/man-db/-/commit/1873051fdb.)
>
> I think these are mostly existing bugs, but I intend to fix them first
> before attempting to land your patch, because this is all delicate
> enough that I really want to be sure of exactly what's changing. (Since
> these are all order-dependent bugs, it may even be that fixing all these
> will make parts of your patch unnecessary; but we'll see.)
>
> I will keep working on this, and expect to be able to get a new release out
> in a week or two.
Thanks a lot for working on this! Once you think you are done, feel free to
ping me in case you'd like me to run my test suite with man-db including all
your changes, making sure that they have the desired effect in the environment
I'm running it
Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
On Sun, May 15, 2022 at 12:20:57AM +0200, Johannes Schauer Marin Rodrigues
wrote:
> I now have a patch (attached).
Thanks for your patch!
I found a few problems with it. Don't worry about sending a new patch
to address these; I have fixes in my local tree for most things I've
commented on here and am working on the rest. I'm just letting you know
where things stand.
First, we should add the appropriate Gnulib modules for upstream
portability. Then:
> @@ -356,8 +356,8 @@ static void add_dir_entries (MYDBM_FILE
>* or . files (such as current, parent dir).
>*/
>
> - dir = opendir (infile);
> - if (!dir) {
> + n = scandir(infile, , NULL, alphasort);
IMO we might as well move the filtering currently being done in the
while loop below to a scandir filter function.
I would prefer not to use alphasort here, because it's locale-dependent
(not that it will matter very much in practice with the sorts of file
names that typically appear in manual page directories, but I can
imagine edge cases). A variant that's deliberately locale-independent
is only a few lines of code.
> @@ -367,13 +367,13 @@ static void add_dir_entries (MYDBM_FILE
>
> /* strlen(newdir->d_name) could be replaced by newdir->d_reclen */
>
> - while ((newdir = readdir (dir)) != NULL) {
> - if (*newdir->d_name == '.' &&
> - strlen (newdir->d_name) < (size_t) 3)
> + while (n--) {
I guess this might have been borrowed from the example in the scandir(3)
manual page, because it goes in reverse order; I don't think there's a
good reason to do that.
> + free(namelist);
This leaks memory. We need to free all the elements of this list as
well.
> order_files (infile, );
This means that the conversion to scandir in this function is in
practice going to be ineffective, because we immediately turn around and
re-sort the list by the physical locations of the first blocks of the
corresponding files. Won't this have just as much of an effect on
reproducibility in principle, even if it doesn't happen to affect
mmdebstrap in your tests, presumably due to something like disk order
typically being similar between runs if your disk isn't too full?
I'm experimenting with simply removing the order_files call here, on the
basis that other performance improvements to mandb(8) have made it less
critical. It does seem to slow things down slightly even on an SSD,
though that may be measurement error; I still need to compare timings on
a rotational drive (but I will).
More interestingly, it changes accessdb(8) output in ways that aren't
just obvious consequences of sorting (multi keys change due to that, but
as far as I can tell that's fine). So far I've spotted the following
issues:
* A number of new entries are introduced for some reason (and the
question is why they were missing beforehand).
* The targets of WHATIS_MAN references change (admittedly in cases
where there are multiple possibilities, but we should be picking a
deterministic one rather than just the first).
* Symlinks flip between ULT_MAN and SO_MAN depending on whether the
symlink happens to sort before its target.
* The database's idea of whether pages require processing via tbl(1)
(and probably other preprocessors) changes in some cases. (Fixed in
https://gitlab.com/cjwatson/man-db/-/commit/1873051fdb.)
I think these are mostly existing bugs, but I intend to fix them first
before attempting to land your patch, because this is all delicate
enough that I really want to be sure of exactly what's changing. (Since
these are all order-dependent bugs, it may even be that fixing all these
will make parts of your patch unnecessary; but we'll see.)
I will keep working on this, and expect to be able to get a new release
out in a week or two.
--
Colin Watson (he/him) [cjwat...@debian.org]
Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Hi Colin, Quoting Johannes Schauer Marin Rodrigues (2022-05-15 00:20:57) > I now have a patch (attached). do you have an approximate ETA for when you think you'll be able to upload a version of man-db that fixes this issue? I'm trying to decide whether I should make a new mmdebstrap release that works around this or wait for a man-db upload that addresses this issue. Thanks! cheers, josch signature.asc Description: signature
Bug#1010957: man-db: unreproducible index.db: contents depend on directory read order
Control: tag -1 + patch
Hi,
Quoting Johannes Schauer Marin Rodrigues (2022-05-14 08:46:48)
> the contents of index.db are unreproducible across different
> hosts/filesystems. With the same host/filesystem, it works fine:
>
> $ export SOURCE_DATE_EPOCH=1652473183
> $ mmdebstrap --variant=standard unstable out1.tar
> $ mmdebstrap --variant=standard unstable out2.tar
> $ cmp out1.tar out2.tar
> $ echo $?
> 0
>
> Now lets mmdebstrap use a filesystem mounted with disorderfs as its
> TMPDIR to simulate the problem:
>
> $ mkdir emptydir disorder
> $ sudo disorderfs --multi-user=yes --shuffle-dirents=yes
> --reverse-dirents=no emptydir disorder
> $ export TMPDIR=$(pwd)/disorder
> $ mmdebstrap --variant=standard unstable out1.tar
> $ mmdebstrap --variant=standard unstable out2.tar
> $ diffoscope out1.tar out2.tar | grep ├──
> ├── file list
> ├── ./var/cache/man/cs/index.db
> ├── ./var/cache/man/da/index.db
> ├── ./var/cache/man/de/index.db
> ├── ./var/cache/man/es/index.db
> ├── ./var/cache/man/fr/index.db
> ...
>
> I attached the contents of /var/cache/man/index.db so that you can see
> that it is indeed the order that differs between individual runs.
I now have a patch (attached).
Thanks!
cheers, josch--- a/src/check_mandirs.c
+++ b/src/check_mandirs.c
@@ -342,8 +342,8 @@ static void add_dir_entries (MYDBM_FILE
{
char *manpage;
int len;
- struct dirent *newdir;
- DIR *dir;
+ int n;
+ struct dirent **namelist;
gl_list_t names;
const char *name;
@@ -356,8 +356,8 @@ static void add_dir_entries (MYDBM_FILE
* or . files (such as current, parent dir).
*/
- dir = opendir (infile);
- if (!dir) {
+ n = scandir(infile, , NULL, alphasort);
+ if (n < 0) {
error (0, errno, _("can't search directory %s"), manpage);
free (manpage);
return;
@@ -367,13 +367,13 @@ static void add_dir_entries (MYDBM_FILE
/* strlen(newdir->d_name) could be replaced by newdir->d_reclen */
- while ((newdir = readdir (dir)) != NULL) {
- if (*newdir->d_name == '.' &&
- strlen (newdir->d_name) < (size_t) 3)
+ while (n--) {
+ if (*namelist[n]->d_name == '.' &&
+ strlen (namelist[n]->d_name) < (size_t) 3)
continue;
- gl_list_add_last (names, xstrdup (newdir->d_name));
+ gl_list_add_last (names, xstrdup (namelist[n]->d_name));
}
- closedir (dir);
+ free(namelist);
order_files (infile, );
@@ -516,8 +516,8 @@ static void fix_permissions_tree (const
static int testmandirs (MYDBM_FILE dbf, const char *path, const char *catpath,
struct timespec last, bool create)
{
- DIR *dir;
- struct dirent *mandir;
+ int n;
+ struct dirent **namelist;
int amount = 0;
bool created = false;
@@ -526,28 +526,28 @@ static int testmandirs (MYDBM_FILE dbf,
if (catpath)
fix_permissions_tree (catpath);
- dir = opendir (path);
- if (!dir) {
+ n = scandir(path, , NULL, alphasort);
+ if (n < 0) {
error (0, errno, _("can't search directory %s"), path);
return 0;
}
if (chdir (path) != 0) {
error (0, errno, _("can't change to directory %s"), path);
- closedir (dir);
+ free(namelist);
return 0;
}
- while( (mandir = readdir (dir)) ) {
+ while (n--) {
struct stat stbuf;
struct timespec mtime;
- if (strncmp (mandir->d_name, "man", 3) != 0)
+ if (strncmp (namelist[n]->d_name, "man", 3) != 0)
continue;
- debug ("Examining %s\n", mandir->d_name);
+ debug ("Examining %s\n", namelist[n]->d_name);
- if (stat (mandir->d_name, ) != 0) /* stat failed */
+ if (stat (namelist[n]->d_name, ) != 0) /* stat failed */
continue;
if (!S_ISDIR(stbuf.st_mode)) /* not a directory */
continue;
@@ -556,14 +556,14 @@ static int testmandirs (MYDBM_FILE dbf,
/* scanned already */
debug ("%s modified %ld.%09ld, "
"db modified %ld.%09ld\n",
- mandir->d_name,
+ namelist[n]->d_name,
(long) mtime.tv_sec, (long) mtime.tv_nsec,
(long) last.tv_sec, (long) last.tv_nsec);
continue;
}
debug ("\tsubdirectory %s has been 'modified'\n",
- mandir->d_name);
+ namelist[n]->d_name);
if (create && !created) {
/* We seem to have something to do, so create the
@@ -577,13 +577,13 @@ static int testmandirs (MYDBM_FILE dbf,
if (errno == EACCES || errno == EROFS) {
debug ("database %s is read-only\n",
dbf->name);
- closedir (dir);
+ free(namelist);
return 0;
} else {
error (0, errno,
_("can't create index cache %s"),
dbf->name);
- closedir (dir);
+ free(namelist);
return -1;
}
}
@@ -592,7 +592,7 @@ static int testmandirs (MYDBM_FILE dbf,
created = true;
} else if (!ensure_db_open (dbf)) {
- closedir (dir);
+ free(namelist);
return 0;
}
@@ -603,14 +603,14 @@ static int testmandirs (MYDBM_FILE dbf,
fprintf (stderr, "\r");
fprintf (stderr,
_("Updating index cache for path "
-
