you asked me to review your next/d-m-h-root branch.
This is what I found:
* Diagnostic messages tend to include DPKG_ROOT. Is that a useful thing
to do?
My intuition was that I'd not include DPKG_ROOT there.
For example:
echo "Obsolete conffile ${DPKG_ROOT}$CONFFILE has been modified by you."
Counter examples:
echo "Restoring backup of $SYMLINK ..."
echo "Restoring backup of $PATHNAME ..."
At the very least, consistency seems in order here.
* In dir_to_symlink, in the case for postrm, in the second (long)
condition, I think there is a line that misses a trailing backslash.
[ \( ! -h "${DPKG_ROOT}$PATHNAME" -a
* In readlink_canon, in the final realpath the $dst argument likely
should be quoted.
* In readlink_canon I was wondering whether passing DPKG_ROOT inside a
sed expression is a good idea. How about reworking the final line:
-realpath $dst | sed -e "s:^$DPKG_ROOT::"
+dst=$(realpath $dst)
+echo ${dst#"$DPKG_ROOT"}
The quoting of the variable removes any pattern meaning from its
contents.
* I observe that readlink -f and readlink_canon significantly differ in
their behaviour. While readlink -f canonicalizes symbolic links in
every component of the path, readlink_canon only canonicalizes
symbolic links in the final component. It then uses realpath saying
that no symlinks are involved, which may be untrue.
I do wonder whether implementing this feature here is a useful thing
to do. It seems that we need a chroot-aware readlink and/or realpath
in more occasions. Possibly extending coreutils is a better solution?
In any case, I'm attaching a version of readlink_canon that deals
with this.
* I observe that the coding style is inconsistent about using test
expressions using -a/-o and &&/||. There is a slight reason to prefer
the latter, see:
https://github.com/koalaman/shellcheck/wiki/SC2166
That's all. Looks like we can go ahead with this with minor changes.
Thanks! I notice this is susceptible to directory traversals. I've
amended it and added comments in the attached version. I'm thinking
I'll need to add unit tests to cover for this among other similar
issues.
On Wed, Apr 29, 2020 at 11:28:08AM +0200, Guillem Jover wrote:
Thanks! I notice this is susceptible to directory traversals. I've
amended it and added comments in the attached version. I'm thinking
I'll need to add unit tests to cover for this among other similar
issues.
I don't think your adaption is correct. Traversing the root directory is actually supported. /../ resolves to /. Returning an error there is not correct.
Sysop: | Keyop |
---|---|
Location: | Huddersfield, West Yorkshire, UK |
Users: | 286 |
Nodes: | 16 (3 / 13) |
Uptime: | 86:10:56 |
Calls: | 6,496 |
Calls today: | 7 |
Files: | 12,099 |
Messages: | 5,277,030 |