[Buildroot] [RFC PATCH v5 02/11] package/patchelf: add patch for rpath sanitation under a root directory

Arnout Vandecappelle arnout at mind.be
Fri Jun 30 22:13:56 UTC 2017


 Hi Wolfgang,

On 30-06-17 10:36, Wolfgang Grandegger wrote:
> The patch allows to use patchelf to sanitize the rpath of the buildroot
> libraries and binaries using the option "--make-rpath-relative <rootdir>".
> 
> Signed-off-by: Wolfgang Grandegger <wg at grandegger.com>

 I still have quite a few remarks, one of which is important: when the same
libfoo.so is present in two directories of the rpath, the second directory
should be removed but it isn't. See below for details.

> ---
>  ...to-make-the-rpath-relative-under-a-specif.patch | 326 +++++++++++++++++++++
>  1 file changed, 326 insertions(+)
>  create mode 100644 package/patchelf/0001-Add-option-to-make-the-rpath-relative-under-a-specif.patch
> 
> diff --git a/package/patchelf/0001-Add-option-to-make-the-rpath-relative-under-a-specif.patch b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-under-a-specif.patch
> new file mode 100644
> index 0000000..609eacf
> --- /dev/null
> +++ b/package/patchelf/0001-Add-option-to-make-the-rpath-relative-under-a-specif.patch
> @@ -0,0 +1,326 @@
> +From af8d4a24a0ef613bdb47f0b1c3d962d59c53a4be Mon Sep 17 00:00:00 2001
> +From: Wolfgang Grandegger <wg at grandegger.com>
> +Date: Mon, 20 Feb 2017 16:29:24 +0100
> +Subject: [PATCH] Add option to make the rpath relative under a specified root
> + directory
> +
> +Running "patchelf" with the option "--make-rpath-relative ROOTDIR" will
> +modify or delete the RPATHDIRs according the following rules
> +similar to Martin's patches [1] making the Buildroot toolchaing/SDK
> +relocatable.
> +
> +RPATHDIR starts with "$ORIGIN":
> +    The original build-system already took care of setting a relative
> +    RPATH, resolve it and test if it's valid (does exist)
> +
> +RPATHDIR starts with ROOTDIR:
> +    The original build-system added some absolute RPATH (absolute on
> +    the build machine). Test if it's valid (does exist).
> +
> +ROOTDIR/RPATHDIR exists:
> +    The original build-system already took care of setting an absolute
> +    RPATH (absolute in the final rootfs), resolve it and test if it's
> +    valid (does exist).
> +
> +RPATHDIR points somewhere else:
> +    (can be anywhere: build trees, staging tree, host location,
> +    non-existing location, etc.). Just discard such a path.
> +
> +The option "--no-standard-libs" will discard RPATHDIRs ROOTDIR/lib and
> +ROOTDIR/usr/lib. Like "--shrink-rpath", RPATHDIRs are also discarded
> +if the directories do not contain a library referenced by the
> +DT_NEEDED fields.
> +If the option "--relative-to-file" is given, the rpath will start
> +with "$ORIGIN" making it relative to the ELF file, otherwise an
> +absolute path relative to ROOTDIR will be used.
> +
> +[1] http://lists.busybox.net/pipermail/buildroot/2016-April/159422.html
> +
> +Signed-off-by: Wolfgang Grandegger <wg at grandegger.com>
> +---
> + src/patchelf.cc | 187 ++++++++++++++++++++++++++++++++++++++++++++++++--------
> + 1 file changed, 161 insertions(+), 26 deletions(-)
> +
> +diff --git a/src/patchelf.cc b/src/patchelf.cc
> +index 55b38e3..60295b3 100644
> +--- a/src/patchelf.cc
> ++++ b/src/patchelf.cc
> +@@ -50,6 +50,9 @@ static int pageSize = PAGESIZE;
> + 
> + typedef std::shared_ptr<std::vector<unsigned char>> FileContents;
> + 
> ++#define MODIFY_FLAG_NO_STD_LIB_DIRS 0x1
> ++#define MODIFY_FLAG_RELATIVE_TO_FILE 0x2
> ++static int modifyFlags;

 I still find it a bad idea to use flags here rather than two separate booleans,
but I guess that's bikeshedding.

> + 
> + #define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Verneed
> + #define ElfFileParamNames Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Verneed
> +@@ -84,6 +87,36 @@ static unsigned int getPageSize()
> +     return pageSize;
> + }
> + 
> ++static bool absolutePathExists(const std::string & path, std::string & canonicalPath)
> ++{
> ++    char *cpath = realpath(path.c_str(), NULL);
> ++    if (cpath) {
> ++        canonicalPath = cpath;
> ++        free(cpath);
> ++        return true;
> ++    } else {
> ++        return false;
> ++    }
> ++}
> ++
> ++static std::string makePathRelative(const std::string & path,
> ++    const std::string & refPath, const std::string & rootDir)
> ++{
> ++    std::string relPath = "$ORIGIN";
> ++
> ++    /* Strip root path first */
> ++    std::string p = path.substr(rootDir.length());
> ++    std::string refP = refPath.substr(rootDir.length());
> ++
> ++    std::size_t pos = refP.find_first_of('/');
> ++    while (pos != std::string::npos) {
> ++        pos =refP.find_first_of('/', pos + 1);
                 ^ missing space

> ++        relPath.append("/..");
> ++    }
> ++    relPath.append(p);

 I probably asked this before already, but why do we need to pass rootDir
instead of just eliminating the common part of path and refPath? Something like:

    while (true) {
        pos = path.find_first_of('/');
        refPos = refPath.find_first_of('/');
        std::string chunk = path.substr(0, pos);
        std::string refChunk = refPath.substr(0, refPos);
        if (chunk != refChunk)
            break;
        path = path.substr(pos);
        refPath = refPath.substr(refPos);
    }
...followed by the loop you already had.

> ++
> ++    return relPath;
> ++}
> + 
> + template<ElfFileParams>
> + class ElfFile
> +@@ -194,9 +227,14 @@ public:
> + 
> +     void setInterpreter(const std::string & newInterpreter);
> + 
> +-    typedef enum { rpPrint, rpShrink, rpSet, rpRemove } RPathOp;
> ++    typedef enum { rpPrint, rpShrink, rpMakeRelative, rpSet, rpRemove} RPathOp;
> + 
> +-    void modifyRPath(RPathOp op, const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath);
> ++    bool libFoundInRPath(const std::string & dirName,
> ++                         const std::vector<std::string> neededLibs);
> ++
> ++    void modifyRPath(RPathOp op,
> ++                     const std::vector<std::string> & allowedRpathPrefixes,
> ++                     std::string rootDir, int flags, std::string newRPath);
> + 
> +     void addNeeded(const std::set<std::string> & libs);
> + 
> +@@ -1108,10 +1146,35 @@ static void concatToRPath(std::string & rpath, const std::string & path)
> +     rpath += path;
> + }
> + 
> ++template<ElfFileParams>
> ++bool ElfFile<ElfFileParamNames>::libFoundInRPath(const std::string & dirName,
> ++    const std::vector<std::string> neededLibs)
> ++{
> ++    std::vector<bool> neededLibFound(neededLibs.size(), false);
> ++
> ++    /* For each library that we haven't found yet, see if it
> ++       exists in this directory. */
> ++    bool libFound = false;
> ++    for (unsigned int j = 0; j < neededLibs.size(); ++j)
> ++        if (!neededLibFound[j]) {
> ++            std::string libName = dirName + "/" + neededLibs[j];
> ++            try {
> ++                if (getElfType(readFile(libName, sizeof(Elf32_Ehdr))).machine == rdi(hdr->e_machine)) {
> ++                    neededLibFound[j] = true;
> ++                    libFound = true;
> ++                } else
> ++                    debug("ignoring library '%s' because its machine type differs\n", libName.c_str());
> ++            } catch (SysError & e) {
> ++                if (e.errNo != ENOENT) throw;
> ++            }
> ++        }
> ++    return libFound;
> ++}
> + 
> + template<ElfFileParams>
> + void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
> +-    const std::vector<std::string> & allowedRpathPrefixes, std::string newRPath)
> ++    const std::vector<std::string> & allowedRpathPrefixes,
> ++    std::string rootDir, int flags, std::string newRPath)

 Since the modifyFlags are anyway global, and since this function is already
using the other global parameter forceRPath, I don't think it makes a lot of
sense to pass flags as an argument. Also, for consistency, it would be better to
still call it modifyFlags IMO.

> + {
> +     Elf_Shdr & shdrDynamic = findSection(".dynamic");
> + 
> +@@ -1162,11 +1225,14 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
> +         return;
> +     }
> + 
> ++    if (op == rpMakeRelative && !rpath) {
> ++        debug("no RPATH to make relative\n");
> ++        return;
> ++    }
> + 
> +     /* For each directory in the RPATH, check if it contains any
> +        needed library. */
> +     if (op == rpShrink) {
> +-        std::vector<bool> neededLibFound(neededLibs.size(), false);

 Darn, I hadn't noticed this before: I think this breaks rpShrink...

 When you have an rpath /xxx:/yyy, and there is a needed library libfoo.so that
exists in both /xxx and /yyy, then rpShrink should still remove /yyy. Indeed,
the one in /xxx gets precedence, and /yyy isn't needed for anything else.

 However, with your refactoring, the neededLibfound array is reset for every
iteration of the rpath loop. So when evaluating /yyy, neededLibFound will again
be false, and /yyy will be kept.

 Actually, come to think of it, the same problem will occur for the
rpMakeRelative, no?

> + 
> +         newRPath = "";
> + 
> +@@ -1186,30 +1252,81 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
> +                 continue;
> +             }
> + 
> +-            /* For each library that we haven't found yet, see if it
> +-               exists in this directory. */
> +-            bool libFound = false;
> +-            for (unsigned int j = 0; j < neededLibs.size(); ++j)
> +-                if (!neededLibFound[j]) {
> +-                    std::string libName = dirName + "/" + neededLibs[j];
> +-                    try {
> +-                        if (getElfType(readFile(libName, sizeof(Elf32_Ehdr))).machine == rdi(hdr->e_machine)) {
> +-                            neededLibFound[j] = true;
> +-                            libFound = true;
> +-                        } else
> +-                            debug("ignoring library '%s' because its machine type differs\n", libName.c_str());
> +-                    } catch (SysError & e) {
> +-                        if (e.errNo != ENOENT) throw;
> +-                    }
> +-                }
> +-
> +-            if (!libFound)
> ++            if (!libFoundInRPath(dirName, neededLibs))
> +                 debug("removing directory '%s' from RPATH\n", dirName.c_str());

 For consistency, we should probably honour --no-standard-lib-dirs here as well.

> +             else
> +                 concatToRPath(newRPath, dirName);
> +         }
> +     }
> + 
> ++    /* Make the the RPATH relative to the specified path */
> ++    if (op == rpMakeRelative) {
> ++        std::string fileDir = fileName.substr(0, fileName.find_last_of("/"));
> ++        newRPath = "";
> ++
> ++        for (auto & dirName : splitColonDelimitedString(rpath)) {
> ++            std::string canonicalPath;
> ++
> ++            /* Figure out if we should keep or discard the path. There are several
> ++               cases to be handled:
> ++               "dirName" starts with "$ORIGIN":
> ++                   The original build-system already took care of setting a relative
> ++                   RPATH. Resolve it and test if it's valid (does exist).
> ++               "dirName" start with "rootDir":
> ++                   The original build-system added some absolute RPATH (absolute on
> ++                   the build machine). Test if it's valid (does exist).
> ++               "rootDir"/"dirName" exists:
> ++                    The original build-system already took care of setting an absolute
> ++                    RPATH (absolute in the final rootfs). Resolve it and test if it's
> ++                    valid (does exist).
> ++               "dirName" points somewhere else:
> ++                    (can be anywhere: build trees, staging tree, host location,
> ++                    non-existing location, etc.). Just discard such a path. */
> ++            if (!dirName.compare(0, 7, "$ORIGIN")) {
> ++                std::string path = fileDir + dirName.substr(7);
> ++                if (!absolutePathExists(path, canonicalPath)) {
> ++                    debug("removing directory '%s' from RPATH because '%s' doesn't exist\n",
> ++			  dirName.c_str(), path.c_str());
> ++                    continue;
> ++                }
> ++            } else if (!dirName.compare(0, rootDir.length(), rootDir)) {
> ++                if (!absolutePathExists(dirName, canonicalPath)) {
> ++                    debug("removing directory '%s' from RPATH because it doesn't exist\n", dirName.c_str());
> ++                    continue;
> ++                }
> ++            } else {
> ++                std::string path = rootDir + dirName;
> ++                if (!absolutePathExists(path, canonicalPath)) {
> ++                    debug("removing directory '%s' from RPATH because it's not in rootdir\n",
> ++			  dirName.c_str());
> ++                    continue;
> ++                }
> ++            }
> ++
> ++            if (flags & MODIFY_FLAG_NO_STD_LIB_DIRS) {
> ++                if (!canonicalPath.compare(rootDir + "/lib") ||
> ++                    !canonicalPath.compare(rootDir + "/usr/lib")) {
> ++                    debug("removing directory '%s' from RPATH because it's a standard library directory\n",
> ++                         dirName.c_str());
> ++                    continue;
> ++                }
> ++            }
> ++
> ++            if (!libFoundInRPath(canonicalPath, neededLibs)) {
> ++                debug("removing directory '%s' from RPATH because it does not contain needed libs\n",
> ++		      dirName.c_str());
> ++                continue;
> ++            }
> ++
> ++            /* Finally make "canonicalPath" relative to "filedir" in "rootDir" */
> ++	    if (flags & MODIFY_FLAG_RELATIVE_TO_FILE)

 Indentation is wrong here (and on the following lines): line starts with tab
instead of spaces.

 Regards,
 Arnout


> ++		concatToRPath(newRPath, makePathRelative(canonicalPath, fileDir, rootDir));
> ++	    else
> ++		concatToRPath(newRPath, canonicalPath.substr(rootDir.length()));
> ++	    debug("keeping relative path of %s\n", canonicalPath.c_str());
> ++	}
> ++    }
> ++
> +     if (op == rpRemove) {
> +         if (!rpath) {
> +             debug("no RPATH to delete\n");
> +@@ -1538,7 +1655,9 @@ static std::vector<std::string> allowedRpathPrefixes;
> + static bool removeRPath = false;
> + static bool setRPath = false;
> + static bool printRPath = false;
> ++static bool makeRPathRelative = false;
> + static std::string newRPath;
> ++static std::string rootDir;
> + static std::set<std::string> neededLibsToRemove;
> + static std::map<std::string, std::string> neededLibsToReplace;
> + static std::set<std::string> neededLibsToAdd;
> +@@ -1561,14 +1680,16 @@ static void patchElf2(ElfFile && elfFile)
> +         elfFile.setInterpreter(newInterpreter);
> + 
> +     if (printRPath)
> +-        elfFile.modifyRPath(elfFile.rpPrint, {}, "");
> ++        elfFile.modifyRPath(elfFile.rpPrint, {}, {}, modifyFlags, "");
> + 
> +     if (shrinkRPath)
> +-        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "");
> ++        elfFile.modifyRPath(elfFile.rpShrink, allowedRpathPrefixes, "", modifyFlags, "");
> +     else if (removeRPath)
> +-        elfFile.modifyRPath(elfFile.rpRemove, {}, "");
> ++        elfFile.modifyRPath(elfFile.rpRemove, {}, "", modifyFlags, "");
> +     else if (setRPath)
> +-        elfFile.modifyRPath(elfFile.rpSet, {}, newRPath);
> ++        elfFile.modifyRPath(elfFile.rpSet, {}, "", modifyFlags, newRPath);
> ++    else if (makeRPathRelative)
> ++        elfFile.modifyRPath(elfFile.rpMakeRelative, {}, rootDir, modifyFlags, "");
> + 
> +     if (printNeeded) elfFile.printNeededLibs();
> + 
> +@@ -1614,6 +1735,9 @@ void showHelp(const std::string & progName)
> +   [--remove-rpath]\n\
> +   [--shrink-rpath]\n\
> +   [--allowed-rpath-prefixes PREFIXES]\t\tWith '--shrink-rpath', reject rpath entries not starting with the allowed prefix\n\
> ++  [--make-rpath-relative ROOTDIR]\n\
> ++  [--no-standard-lib-dirs]\n\
> ++  [--relative-to-file]\n\
> +   [--print-rpath]\n\
> +   [--force-rpath]\n\
> +   [--add-needed LIBRARY]\n\
> +@@ -1674,6 +1798,17 @@ int mainWrapped(int argc, char * * argv)
> +             setRPath = true;
> +             newRPath = argv[i];
> +         }
> ++        else if (arg == "--make-rpath-relative") {
> ++            if (++i == argc) error("missing argument to --make-rpath-relative");
> ++            makeRPathRelative = true;
> ++            rootDir = argv[i];
> ++        }
> ++        else if (arg == "--no-standard-lib-dirs") {
> ++            modifyFlags |= MODIFY_FLAG_NO_STD_LIB_DIRS;
> ++        }
> ++        else if (arg == "--relative-to-file") {
> ++            modifyFlags |= MODIFY_FLAG_RELATIVE_TO_FILE;
> ++        }
> +         else if (arg == "--print-rpath") {
> +             printRPath = true;
> +         }
> +-- 
> +1.9.1
> +
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF


More information about the buildroot mailing list