[Rpm-maint] [PATCH] Unbreak short-circuited binary builds

[Mark Wielaard]

On Thu, Jan 05, 2017 at 03:40:42PM +0200, Panu Matilainen wrote:
> > > When checking for pre-existing links see if they already point to
> > > the right file and in that case just reuse it instead of creating new ones.
> > 
> > > @@ -1572,6 +1573,16 @@ static int addNewIDSymlink(FileList fl,
> > >      origpath = linkpath;
> > > 
> > >      while (faccessat(AT_FDCWD, linkpath, F_OK, AT_SYMLINK_NOFOLLOW) == 0) {
> > > +	char ltarget[PATH_MAX];
> > > +	ssize_t llen;
> > > +	/* In short-circuited builds the link might already exist  */
> > > +	if ((llen = readlink(linkpath, ltarget, sizeof(ltarget)-1)) != -1) {
> > > +	    ltarget[llen] = '\0';
> > > +	    if (rstreq(ltarget, targetpath)) {
> > > +		exists = 1;
> > > +		break;
> > > +	    }
> > > +	}
> > 
> > Allocating PATH_MAX size on the stack might be a bit much
> > (and on some systems PATH_MAX might not exist). Might be
> > good to use lstat first to see if it is a symbolic link
> > and use the st_size i+1 as buffer size. I would malloc/free
> > it to not accidentially blow up the stack.
> 
> Yeah, using PATH_MAX is broken but then rpm is getting away with it (on
> stack and all) in many other places so one more here and there... but good
> point using lstat() there, will change it to use that, thanks.

Ah, indeed. rpm does allocated PATH_MAX bufs on the stack already.
O well. One day we will clean this all up.

> Other than that - does it look like doing the right thing to you? I'm really
> not familiar with the debuginfo business at all :)

You made me look at the whole thing in context!
(Spoiler. Yes, I think the patch is correct. But first I am going
to bore you by explaining the whole function and why it is called
in the first place.)

OK. addNewIDSymlink () is called from generateBuildIDs () at the
end of processPackageFiles () for the FileList. It is called for
each (regular) loadable ELF image file that contains a build-id.

The symlinks are named after the (hex) build-id so they actual
ELF image file associated with the build-id can be easily found.
But they are not all put in the same directory, the first two
hex-digits are used as sub-directory say they are spread out
over 255 different directories. There are two kinds of build-id
symlinks. Those that point to the actual ELF image and those
(ending in .debug) pointing at the associated separate debuginfo
file.

Both kind of build-id files used to be put together under
/usr/lib/debug/.buildid/... directories in the debuginfo
package. But this causes strange issues when the debuginfo
package isn't installed, if the main package and debuginfo
package are out of sync or if the debuginfo package is
installed, but the main package isn't.

So with the latest rpm the main build-id files are put in
their own separate directories in the main (sub)-package
under /usr/lib/.build-id/ with the .debug build-ids still in
the -debuginfo package under /usr/lib/debug/.build-id/. There
is also a compatibility setting %_build_id_links compat that
puts an extra symlink under /usr/lib/debug/.build-id that
just points to the /usr/lib/.build-id symlink. See the comments
in macros.in for the different settings for %_build_id_links.

So addNewIDSymlink () is called with the FileList, the target
(which is the actual ELF file), the idlinkpath (which includes
the whole prefix and 2-digit hex subdir) and whether a debug
or compat link is requested. If it is a debug build-id link
then .debug is added to the idlinkpath.

The build-ids should be globally unique. Some of my recent
patches makes sure that is always the case between packages
even if they differ only very slightly by hashing in the nvr
into the build-id put in the ELF images (through debugedit).
But a package could still generate two identical ELF images
by having the the same ELF file in the package (either hard
linked or copied). So then addNewIDSymlink checks (in a loop)
if the idlinkpath already exists. If so it adds a counter
(.<nr>) to the linkpath.  If it is a compat link then both
the target and link get the same counter.

Then, since duplicate build-ids really shouldn't exist, if
this wasn't a compat link, a warning is generated about the
files that contain the same build-id unless the files were
hard links.

So summing up the above I think your patch is correct.
The loop that checks if the idlinkpath already exists should
indeed check that if it exists it points to a different target
than expected. Normally this would indeed never happen.
Or can a FileList contain the same file more than once?
But in a short-circuited build it will already have been
created.

I would just change the comment to explicitly say
"Check if the symlink already points to the expected target."

Cheers,

Mark