[SOLVED] Bug in makepkg and symlinks with blanks in filename?

DarkVision · 10-21-2013, 12:45 AM

Quote:

Originally Posted by ruario

Yep, but those files are removed with the patched removepkg, right?

Yes, of course. The patched removepkg works fine with those links. I just wanted to have a look what happens if someone installs a "new" package on a system with old removepkg. If those links with spaces are the only files in a directory to be removed you get a warning that there are "new" files in these directories (the files with spaces in the filename).

I think that is OK and much better then install broken symlinks with an unpatched makepkg.

ruario · 10-21-2013, 01:44 AM

Quote:

Originally Posted by gnashley

I am a bit surprised that Patrick expressed any desire to change from ';' to using '&&'. I figured that the use of ';' was kept so that it would be obvious when link-creation lines were failing, by leaving the faulty links in the root of '/'.

Consider however that a failing cd command also means that the rm command after ';' is executed in the root of '/' as well. If there is a name clash, then one of the root directories and all of its subdirectories and files would be deleted. Therefore the advantage in using && to prevent accidental removal, is far greater than the side benefit that it leaves the broken symlinks in root, for the packager to find.

If time could be turned back then && would be much better, IMHO. It is up to Patrick however to decide is it is worth doing now. I provided a regex for removepkg to work with both methods but there may be other scripts that could break as they do not expect this.

ruario · 10-21-2013, 04:27 AM

An example of how changes to the symlink shell script formatting can break things. I just tested Spkg (a re-implemented of installpkg, upgradepkg and removepkg in C, optimised for speed). Whilst not installed on Slackware itself by default, I know that some people do use it. Additionally it used by some Slackware derivatives. For example, Salix installs it (alongside the official pkgtools) and most Salix users would use it by default as their slapt-get version uses it in place of the original Pkgtools. Spkg naturally expects the current formatting conventions in symlink shell script code in doinst.sh. Adding support for -- and && results in uninstall of all symlinks failing.

If I use a patched makepkg to produce packages that contain some symlinks with escaped spaces in their names, then Spkg fails to remove those symlinks but the rest are still removed correctly. So this reaffirms (to me at least) that the more cautious, minimal change of only using escaping when needed causes the least problems for current tools that might already be out there.

EDIT: To play devil's advocate, I suppose a counter argument is that if you change the format and all third party tools expecting the current format break in all cases, those tools are likely to be fixed sooner as their author will probably notice immediately. If you allow one small change (space escaping) those maintaining other tools may not notice for years as spaces in file names are not common in *nix, so the author might not experience the issue.

gnashley · 10-21-2013, 09:32 AM

I wasn't advocating for either 'ln -fs' or the use, or not, of '&&' instead of ';'.

DarkVision · 10-21-2013, 01:40 PM

OK... looks like it's not that easy (3rd-party apps need to be fixed) so for now i will use ruarios find/sed code in slackbuilds and remove any symlinks before i call makepkg if the package contain symlinks with spaces.

If i don't do that and a user want to build a slackbuild on a system with an unpatched makepkg he will get a corrupt doinst.sh script (even if that may happen for about one in a million packages only). On the other side the only problem may be that removepkg will not be able to remove symlinks with spaces and that other 3rd-party tools may not be able to handle that package correct. Well... this is something i can not take care about.

I'm sure patching makepkg and removepkg may be the better solution, but for now slackbuild maintainers must check for spaces in symlinks and apply a workaround to the script since they can not be sure that the package will be compiled on a system with a patched makepkg. As mentioned above a corrupt script could remove existing files if parts of the symlink filename match other files in the same directory.

Right now we were talking about symlinks with spaces in the filename. I checked what will happen with an unpatched makepkg if you create a symlink inside a directory with spaces in the directory name. Here is my test script:

Code:

#!/bin/sh
TMPDIR=test-$(mcookie)
mkdir -p "${TMPDIR}/usr/bin"
mkdir -p "${TMPDIR}/usr/share/test dir"
mkdir -p "${TMPDIR}/usr/share/other_dir"

# Cretae a symlink with spaces in the filename
touch "${TMPDIR}/usr/bin/This is a filename with spaces"
ln -sf "This is a filename with spaces" "${TMPDIR}/usr/bin/This is a symlink with spaces"

# Create a symlink inside a directory with spaces in the directory name
touch "${TMPDIR}/usr/share/other_dir/testlink"
ln -sf "../other_dir/testlink" "${TMPDIR}/usr/share/test dir/testlink"

echo "Make symlinks in doinst.sh using find..."
( cd "${TMPDIR}"
  # Add -exec  rm -f '{}' \; to remove the links whuile creating the doinst.sh
  find * -type l -printf '%p/*>/%l\n' | sed 's, ,/*/,g;s,/\*>/, -> ,;s,/\*/,\\ ,g' |\
    sed 's,\(.*\)/\(.*\) -> \(.*\),( cd \1 \; rm -rf \2 )\n( cd \1 \; ln -sf \3 \2 ),'
)

echo "Make symlinks in doinst.sh using makepkg..."
( cd "${TMPDIR}"
  makepkg -l y -c n "../test-1.0-noarch.tgz" &>/dev/null
)
cat "${TMPDIR}/install/doinst.sh"

And here is the result: Compare the directories...

Quote:

Make symlinks in doinst.sh using find...
( cd usr/bin ; rm -rf This\ is\ a\ symlink\ with\ spaces )
( cd usr/bin ; ln -sf This\ is\ a\ filename\ with\ spaces This\ is\ a\ symlink\ with\ spaces )
( cd usr/share/test\ dir ; rm -rf testlink )
( cd usr/share/test\ dir ; ln -sf ../other_dir/testlink testlink )
Make symlinks in doinst.sh using makepkg...
( cd usr/bin ; rm -rf This )
( cd usr/bin ; ln -sf a This )
( cd usr/share ; rm -rf test )
( cd usr/share ; ln -sf -> test )

The target directory changes from "usr/share/test dir" to "usr/share" which might DELETE files with the name "test" in a totally different directory. EDIT: ruarios code seem to handle that very well... i also tested some other scenarios like a linked directory with spaces in the name. The find code seem to work for everything. /EDIT

As i said above... i will check for spaces and use ruarios code to create a doinst.sh manually for the slackbuilds affected by that. So this is temporary solved for me right now. Thanks a lot for the help ruario

P.S. To give a more "real world" example, here is another script:

Code:

#!/bin/sh
TMPDIR=test-$(mcookie)
mkdir -p "${TMPDIR}/usr/share/themes/Clearlooks Compact"

# Create a symlink inside a directory with spaces in the directory name
touch "${TMPDIR}/usr/share/themes/Clearlooks Compact/index.theme"
ln -sf "index.theme" "${TMPDIR}/usr/share/themes/Clearlooks Compact/index.theme.orig"

echo "Make symlinks in doinst.sh using find..."
( cd "${TMPDIR}"
  # Add -exec  rm -f '{}' \; to remove the links while creating the doinst.sh
  find * -type l -printf '%p/*>/%l\n' | sed 's, ,/*/,g;s,/\*>/, -> ,;s,/\*/,\\ ,g' |\
    sed 's,\(.*\)/\(.*\) -> \(.*\),( cd \1 \; rm -rf \2 )\n( cd \1 \; ln -sf \3 \2 ),'
)

echo "Make symlinks in doinst.sh using makepkg..."
( cd "${TMPDIR}"
  makepkg -l y -c n "../test-1.0-noarch.tgz" &>/dev/null
)
cat "${TMPDIR}/install/doinst.sh"

Here is the Output:

Code:

Make symlinks in doinst.sh using find...
( cd usr/share/themes/Clearlooks\ Compact ; rm -rf index.theme.orig )
( cd usr/share/themes/Clearlooks\ Compact ; ln -sf index.theme index.theme.orig )
Make symlinks in doinst.sh using makepkg...
( cd usr/share/themes ; rm -rf Clearlooks )
( cd usr/share/themes ; ln -sf -> Clearlooks )

Install clearlooks-compact with a link in the packages theme top level directory would delete a directory named Clearlooks and i have currently both themes installed. So after i installed package clearlooks-compact i would miss the clearlooks directory.

Yes, i know... all examples here are more or less constructed, may only appear once in a million.

Lennie · 10-21-2013, 03:53 PM

Why the use of the recursive flag when deleting symlinks?

Code:

( cd path/to/somewhere ; rm -rf somelink )

Symlinks are files, not directories. Even worse, if you run 'rm -rf linkname/' then it'll remove everything in the directory it links to, but the symlink will still be there.

Or is the purpose really to remove whatever happened to be in the way, even if it is a directory? The total lack of security is amazing... Test if it is a link, and only then remove it. Otherwise inform the user about which link couldn't be made and needs to be made manually.

ruario · 10-21-2013, 04:01 PM

It is intentionally to allow a symlink to a replace a directory. 'rm -rf linkname/' (with the final slash) will not be created by the shell script creation code, so this is irrelevant.

The "security" is the person doing the packaging. He/She need to make sure the package does what it should and no more. All official packages are built by Pat. I think we can trust him. Any extra packages should only come from a reputable source or you should make them (and hence check them yourself).

ruario · 10-21-2013, 04:21 PM

Just to be clear, on post install rm -rf somelink is used to clear stuff out of the way prior creating the symlink. However on uninstall of the package -r is not used, it would be deleted as rm -f path/to/somewhere/somelink and only if it is not found in some other package.

volkerdi · 10-21-2013, 04:25 PM

Quote:

Originally Posted by Lennie

Why the use of the recursive flag when deleting symlinks?

Code:

( cd path/to/somewhere ; rm -rf somelink )

Symlinks are files, not directories. Even worse, if you run 'rm -rf linkname/' then it'll remove everything in the directory it links to, but the symlink will still be there.

Or is the purpose really to remove whatever happened to be in the way, even if it is a directory?

That is it entirely. There have been quite a few cases in the past where a symlink needs to replace an existing directory.

Quote:

The total lack of security is amazing... Test if it is a link, and only then remove it. Otherwise inform the user about which link couldn't be made and needs to be made manually.

Given the reason that this is done, I don't see a better way of doing it. Lack of security... seriously?

DarkVision · 10-21-2013, 11:27 PM

Quote:

Originally Posted by Lennie

The total lack of security is amazing...

I also do not see any security issues here...

Quote:

Originally Posted by ruario

The "security" is the person doing the packaging. He/She need to make sure the package does what it should and no more. All official packages are built by Pat. I think we can trust him. Any extra packages should only come from a reputable source or you should make them (and hence check them yourself).

The 'Problem' are not 3rd-party packages... the problem are 3rd-party slackbuilds which are compiled by a normal user who might not be aware of this 'problem'. So the slackbuild maintainer must check that the doinst.sh script is generated with correct paths to directories and symlinks. If there are no spaces then everything is fine. If there are spaces he also need to test on a system without a patched makepkg. And that's why i can not rely on makepkg working well, at least for now. I currently have two affected packages only, easy to handle.

ruario · 10-22-2013, 02:11 AM

3rd-party slackbuilds are exactly the same. The person who writes it has a responsibility to check that it works as expected. Given that almost nobody (perhaps just you and me thus far) will have patched their makepkg/removepkg to handle symlinks with spaces in their paths, the slackbuild author should assume that they will not be handled and find some way to work around this. There are several possible work-arounds e.g. hard link instead, copy the file so that a symlink is not needed, rename the file or directory (assuming this will not break how the package works), delete them (if the particular files are not essential), etc. The packager might also want to ask upstream if they can change the source package to be more UNIX-like in its file naming conventions, thus eliminating the issue.

Before they start packaging the SlackBuild creator can check for spaces in symlink paths in the original source package with a command like the following:

Code:

find . -type l -printf '%p>%l\n' | grep ' '

If they get a result then they could employ a work-around or discuss with upstream about the feasibility of changing the source package.

If at some later stage Pat starts shipping Slackware with a makepkg and removepkg that can handle symlinks with spaces in their path, the slackbuild creator/maintainer may choose to remove any workaround they have implemented (though this would make their Slackbuild will only compatible with that version of Slackware and above, so they might want to leave it in place if they care about users of older versions of Slackware).

Anyway, I wouldn't expect an immediate response from volkerdi on how he intends to handle this right now, since 14.1 is just about to release. I think it is unlikely that he would make changes to pkgtools at this late stage, so any fix (if there is one) will likely roll into current after 14.1 ships. Which means he doesn't need to spend too much time thinking about this right away.

DarkVision · 10-22-2013, 02:54 AM

Quote:

Originally Posted by ruario

3rd-party slackbuilds are exactly the same. The person who writes it has a responsibility to check that it works as expected.
...

Anyway, I wouldn't expect an immediate response from volkerdi on how he intends to handle this right now, since 14.1 is just about to release...

Sure, the author must make sure that the slackbuild works as expected, but once you are working with a fixed makepkg you still have to make sure the slackbuild will work on older systems with the original makepkg also or you leave a note that older systems are not supported. I think that problem is that rare that not many people ever had that problem and therefore have not checked for that.

Anyway i agree with this comment:

Quote:

Originally Posted by rkfb

If you write a program that accepts input and someone throws input at it that it can't handle then the sensible solution must be to adjust your program accordingly. Even if, as in this scenario, the program exits and says 'Please fix the spaces in the filenames'.

I'm sure there won't be a quick fix, that's why i use the original version of makepkg shipped by slackware and modify my slackbuilds to work with these versions. Kind of a workaround for a 'bug' in makepkg but i don't see any other solution right now. Rename the files and patch the applications is for sure no choice...

GazL · 10-22-2013, 04:21 AM

Quote:

Originally Posted by Lennie

Why the use of the recursive flag when deleting symlinks?

Code:

( cd path/to/somewhere ; rm -rf somelink )

Symlinks are files, not directories. Even worse, if you run 'rm -rf linkname/' then it'll remove everything in the directory it links to, but the symlink will still be there.

Sure about that are you?

Code:

gazl@ws1:/tmp$ mkdir wibble
gazl@ws1:/tmp$ cd wibble
gazl@ws1:/tmp/wibble$ mkdir wobble
gazl@ws1:/tmp/wibble$ echo "plop" > wobble/bobble.txt
gazl@ws1:/tmp/wibble$ ln -sf wobble link
gazl@ws1:/tmp/wibble$ ls -lR
.:
total 0
drwxr-xr-x 2 gazl users 60 Oct 22 09:26 wobble
lrwxrwxrwx 1 gazl users  6 Oct 22 09:27 link -> wobble

./wobble:
total 4
-rw-r--r-- 1 gazl users 5 Oct 22 09:26 bobble.txt
gazl@ws1:/tmp/wibble$ rm -rf link
gazl@ws1:/tmp/wibble$ ls -lR
.:
total 0
drwxr-xr-x 2 gazl users 60 Oct 22 09:26 wobble

./wobble:
total 4
-rw-r--r-- 1 gazl users 5 Oct 22 09:26 bobble.txt
gazl@ws1:/tmp/wibble$

doinst.sh runs as root, so a malicious package doesn't need to play silly games with symlinks in order to wreak havok, so this approach doesn't make the situation any worse than it already is from a security standpoint.

A discussion about the most robust approach to handling a package that contains a symlink replacing an existing non-empty directory is something that is likely to have many facets. I believe that other packaging systems handle this sort of thing by throwing out packaging conflict errors, aborting the package install and requiring the end user to resolve them manually. Slackware's approach is not to burden the end user with this and just to clobber the existing stuff. Which is better is a matter of opinion.

P.S. The safest way to replace an existing empty directory with a symlink is "ln -Tsf", no need for a "rmdir" or "rm -rf" first.

ruario · 10-22-2013, 04:42 AM

@DarkVision: As a side note, just by having started this thread you have gone some way to alleviating the problem. volkerdi and AlienBob have commented in the thread and hence know about the issue, so will probably be checking for spaces in symlink paths for any new packages they choose to maintain (assuming of course they hadn't been doing this already). Additionally with 1,550 views to this thread thus far, I'd be surprised if one or more of the SBo team haven't seen it as well, so they can keep an eye on new submissions.

Perhaps gnashley might want to consider adding a warning/error to src2pkg (and/or pkg-check) if you try to make a package with spaces in the symlink path?

ruario · 10-22-2013, 06:21 AM

Quote:

Originally Posted by rkfb

If you write a program that accepts input and someone throws input at it that it can't handle then the sensible solution must be to adjust your program accordingly. Even if, as in this scenario, the program exits and says 'Please fix the spaces in the filenames'.

So you are suggesting something like the following to be added to makepkg as an interim fix until a full solution can be tested and applied?

Code:

if find * -type l -printf '%p>%l\n' | grep -q ' '; then
  echo "This package contains the following symlinks with spaces in their path" >&2
  echo -e "or the path of the object they point to:\n" >&2
  find * -type l -printf '%p -> %l\n' >&2
  echo -e "\nPlease correct or remove the above before you continue." >&2
  exit 1
fi

The only problem I would have with this is that makepkg does very little in terms of sanity checking thus far, so you are changing the scope of the program. If this is accepted, what other checks should be added? I think this kind of code makes more sense in a separate package checking program.