Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 446 - sys-apps/portage - Implement a sanity check on symlinks in ${D}.
Summary: sys-apps/portage - Implement a sanity check on symlinks in ${D}.
Status: RESOLVED FIXED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Enhancement/Feature Requests (show other bugs)
Hardware: All Linux
: Low normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords: InVCS, PATCH
Depends on:
Blocks: 462382
  Show dependency tree
 
Reported: 2002-01-30 17:35 UTC by Leo Lipelis (RETIRED)
Modified: 2018-10-07 20:30 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
portage_symlink_qa_warning.patch (file_446.txt,688 bytes, text/plain)
2013-03-15 11:46 UTC, Tom Wijsman (TomWij) (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Lipelis (RETIRED) gentoo-dev 2002-01-30 17:35:03 UTC
It is important to perform sanity checks on any and all symlinks created in
${D}.  It's possible that a "make install" or a similar script might create
invalid symlinks, especially when building .so libraries.

I believe this was originally suggested by Vitaly, but somehow it got lost in
the ethers :).
Comment 1 Daniel Robbins (RETIRED) gentoo-dev 2002-02-21 01:10:52 UTC
um, what constitutes an invalid symlink?  What specifically should we be
checking for?
Comment 2 Daniel Robbins (RETIRED) gentoo-dev 2002-03-19 16:33:00 UTC
we now merge symlinks in the proper order to avoid brokenness; but we *do* allow
the merging of broken symlinks and this is needed for some packages and is ok.
Comment 3 Christoph Junghans gentoo-dev 2011-07-21 20:05:50 UTC
After zac fixed one of my ebuilds I was wondering why there is no qa message if a package installs a broken symlink.

Should we rethink this?
Comment 4 Zac Medico gentoo-dev 2011-07-21 21:05:10 UTC
There's no guarantee that they'll resolve correctly until they're installed, so the check would probably have to be delayed until after the package is installed.

Also, once they're installed, if $ROOT != / then they'll only resolve correctly if they're written as relative symlinks like ../../usr/lib/foo or whatnot. We might want to add something to PMS to require that all symlinks are relative rather than absolute.
Comment 5 Zac Medico gentoo-dev 2011-07-21 21:35:00 UTC
If we assume that all symlinks resolve to a target that's installed by the package itself, then we can implement a sanity check that performs simulated symlink resolution for absolute symlinks inside $D. Maybe this assumption is good enough to avoid false positives in the vast majority of cases.

It's come to my attention that python.eclass has a working implementation of a check like this inside the python_merge_intermediate_installation_images() function, where it resolves the simulated target of a symlink and stores it in the ${absolute_file} variable.
Comment 6 Zac Medico gentoo-dev 2011-07-21 21:46:53 UTC
(In reply to comment #5)
> If we assume that all symlinks resolve to a target that's installed by the
> package itself

We can also simulate resolution in $ROOT, so we don't need to make this assumption. That should eliminate nearly all false positives.

I'll remove pms-bugs from CC since it appears that it's not really necessary to involve PMS.
Comment 7 Christoph Junghans gentoo-dev 2011-07-24 15:11:53 UTC
(In reply to comment #5)
> If we assume that all symlinks resolve to a target that's installed by the
> package itself, then we can implement a sanity check that performs simulated
> symlink resolution for absolute symlinks inside $D. Maybe this assumption is
> good enough to avoid false positives in the vast majority of cases.
That's sound good to me. There are only very few packages which create a symlink to something outside it's own files. And there should be something like QA_BROKEN_SYMLINK to skip the check for some files.
Comment 8 Tom Wijsman (TomWij) (RETIRED) gentoo-dev 2013-03-15 11:46:35 UTC
Created attachment 342118 [details]
portage_symlink_qa_warning.patch

Proposed patch for this old bug that we have to resolve one or another day (because it's a good QA idea), I'm new to Portage development so I am not sure if this is the right approach but it would serve as a start; just checking if it is on root or in the destination directory should be sufficient, I don't know of any special cases and I assume there are none unless you tell me otherwise.

It took me some time to find where to insert this and I have done it here because it seemed the most easy approach to me (but not necessarily the right one), if you want this to happen before or after the actual merge then please point me to where in the code this happens; the code structure is still quite unclear to me (and the documentation isn't helping me).

In this patch this is a warning such that developers are at least aware of this. We should however look at whether there is a valid case for a symlink to be invalid, otherwise it would be better to turn this into an error. As long as there are no valid cases for which it makes sense, I don't see a need for a QA variable.

Something to keep in mind to the future might be that we need to refactor the code such that we queue every file to be installed through some "checking" functions; maybe that happens already at the moment, but if that's the case then I simply saw no such thing in the limited search time I spent on it. But I'm just saying that having all these special conditions, checks, messages, warnings, errors and other magic in the code as it is now; makes it hard to maintain in the long run.
Comment 9 Zac Medico gentoo-dev 2013-03-16 02:46:15 UTC
(In reply to comment #8)
> Created attachment 342118 [details]
> portage_symlink_qa_warning.patch
> 
> Proposed patch for this old bug that we have to resolve one or another day
> (because it's a good QA idea), I'm new to Portage development so I am not
> sure if this is the right approach but it would serve as a start; just
> checking if it is on root or in the destination directory should be
> sufficient, I don't know of any special cases and I assume there are none
> unless you tell me otherwise.

I think that should work correctly.

> It took me some time to find where to insert this and I have done it here
> because it seemed the most easy approach to me (but not necessarily the
> right one), if you want this to happen before or after the actual merge then
> please point me to where in the code this happens; the code structure is
> still quite unclear to me (and the documentation isn't helping me).

That place should be fine for now. If we decide to make it an error, then we should do it before the merging starts, like the collision-protect code.
 
> In this patch this is a warning such that developers are at least aware of
> this. We should however look at whether there is a valid case for a symlink
> to be invalid, otherwise it would be better to turn this into an error. As
> long as there are no valid cases for which it makes sense, I don't see a
> need for a QA variable.

Yeah, we can do that after we've tested the warning for awhile.

> Something to keep in mind to the future might be that we need to refactor
> the code such that we queue every file to be installed through some
> "checking" functions; maybe that happens already at the moment, but if
> that's the case then I simply saw no such thing in the limited search time I
> spent on it. But I'm just saying that having all these special conditions,
> checks, messages, warnings, errors and other magic in the code as it is now;
> makes it hard to maintain in the long run.

Yeah, that's a good idea. We don't have anything like that yet.
Comment 10 Zac Medico gentoo-dev 2013-03-16 05:41:47 UTC
Your patch is in git:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=ca7467006b112f65a57c4db7816729841a2de694

I've modified it to use lexists, sine if the target happens to be a broken symlink then that should trigger an independent warning:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=8cfe2dc28e02a9cdd739a83db4ed871a31468114
Comment 11 Zac Medico gentoo-dev 2013-03-22 02:50:17 UTC
This is fixed in 2.1.11.56 and 2.2.0_alpha167.