libtrace.c: use realpath instead of readlink to avoid PATH_MAX

[kmk3]

PATH_MAX is not guaranteed to be defined and it may be defined to -1.
Avoid depending on it by getting the result directly from realpath. See
commit 579f856 ("firejail.h: add missing linux/limits.h include") /
PR #4583 for details.

Note: This replaces the static char array currently used with a dynamic
one returned from realpath.

Misc: This is a continuation of #4583.

[kmk3]

By the way, I noticed that there is barely any dynamic memory being allocated
on libtrace.c. Is it because of performance or security? Could realpath be
a problem in this case? If so, then it might be better to just define a
PATH_MAX fallback (i.e.: if not defined) instead.

[smitsohu]

I don't think there are any security or performance implications. LGTM!

(If you are asking for nitpicks: I think readlink is a tad more robust, as it works even if the executable doesn't exist anymore in the current mount namespace. Also realpath is more likely to fail if there is memory pressure, which can be a feature of the sandbox, thinking of setrlimit-as... but I think that users will hardly run into anything of this).

[kmk3]

@smitsohu commented on Oct 16:

I don't think there are any security or performance implications. LGTM!

Great.

(If you are asking for nitpicks:

Yes, especially when touching things that I don't know much about, such as
tracing.

I think readlink is a tad more robust, as it works even if the executable
doesn't exist anymore in the current mount namespace.

Interesting, I had no idea about this.

Also realpath is more likely to fail if there is memory pressure, which can
be a feature of the sandbox, thinking of setrlimit-as... but I think that
users will hardly run into anything of this).

I see; would it be an improvement to check for ENOMEM and exit or would that
just make it more verbose?

diff --git a/src/libtrace/libtrace.c b/src/libtrace/libtrace.c
index 106ce99f3..98d8603f2 100644
--- a/src/libtrace/libtrace.c
+++ b/src/libtrace/libtrace.c
@@ -18,6 +18,7 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 */
 #define _GNU_SOURCE
+#include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -706,7 +707,11 @@ static void log_exec(int argc, char** argv) {
	(void) argc;
	(void) argv;
	char *buf = realpath("/proc/self/exe", NULL);
-	if (buf != NULL) {
+	if (buf == NULL) {
+		if (errno == ENOMEM) {
+			tprintf(ftty, "realpath: %s\n", strerror(errno));
+			exit(1);
+		}
+	} else {
		tprintf(ftty, "%u:%s:exec %s:0\n", mypid, myname, buf);
		free(buf);
	}

Besides not having to deal with PATH_MAX directly, I also thought that this
could possibly (untested) allow tracing paths bigger than PATH_MAX, which
would be kind of interesting.

Note: If the downsides of realpath are bigger than the upsides, this doesn't
really have to be merged. I have a branch that organizes the limits.h includes
and that sets hardcoded fallbacks to ARG_MAX/PATH_MAX (and that conflicts
with this branch), which I'm more interested in anyway. I submitted the
realpath changes separately as they seem a bit more unusual compared to the
other ones.

[smitsohu]

Besides not having to deal with PATH_MAX directly, I also thought that this
could possibly (untested) allow tracing paths bigger than PATH_MAX, which
would be kind of interesting.

As far as I know these magic links from /proc are never longer than 4096 bytes, which is the size of a single page. People hardcode this frequently.

Well... let's not complicate it too much. It looks like a simple change, whatever way you choose will be right.

[kmk3]

@smitsohu commented on Oct 16:

Besides not having to deal with PATH_MAX directly, I also thought that this
could possibly (untested) allow tracing paths bigger than PATH_MAX, which
would be kind of interesting.

As far as I know these magic links from /proc are never longer than 4096
bytes, which is the size of a single page. People hardcode this frequently.

Well... let's not complicate it too much. It looks like a simple change,
whatever way you choose will be right.

Alright, I did a force-push with the changes from the diff; unless any other
points are raised, that would be the final version.

[smitsohu]

Merged.