syscall_emul: Bandage readlink /proc/self/exe

Information
Submitter:	Joel Hestness
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11092:3f4435aa225e
---------------------------
syscall_emul: Bandage readlink /proc/self/exe

The recent changeset to readlink() to handle reading the /proc/self/exe link
introduces a number of problems. This patch fixes two:

1) Because readlink() called on /proc/self/exe now uses LiveProcess::progName()
to find the binary path, it will only get the zeroth parameter of the simulated
system command line. However, if a config script also specifies the process'
executable, the executable parameter is used to create the LiveProcess rather
than the zeroth command line parameter. Thus, the zeroth command line parameter
is not necessarily the correct path to the binary executing in the simulated
system. To fix this, add a LiveProcess data member, 'executable', which is
correctly set during instantiation and returned from progName().

2) If a config script allows a user to pass a relative path as the zeroth
simulated system command line parameter or process executable, readlink() will
incorrecly return a relative path when called on '/proc/self/exe'.
/proc/self/exe is always set to a full path, so running benchmarks can fail if
a relative path is returned. To fix this, clean up the handling of
LiveProcess::progName() within readlink() to get the full binary path.

NOTE: This patch still leaves the potential problem that host full path to the
binary bleeds into the simulated system, potentially causing apparent
non-deterministic simulated system execution.

Testing Done

Issue Summary

Description	From	Last Updated	Status
This comment is inaccurate, and therefore may lead to some confusion. There is no non-determinism, apparent or otherwise. I think ...	Tony Gutierrez	Sept. 29, 2015, 7:43 a.m.	Resolved

I've had the same problem with the absolute vs. relative path thing, and had just been working around it by specifying an absolute path on the command line. Nice to have a real fix for this, but I'm not convinced by it :).

Joel Hestness Sept. 17, 2015, 12:47 p.m. (Sept. 17, 2015, 12:47 p.m.)

Thanks for the pointers. The updated patch addresses both - hopefully more convincing.

src/sim/process.cc (Diff revision 1)

why not just reassign params->executable here? then we could get rid of the redundant test in the constructor above

src/sim/syscall_emul.cc (Diff revision 1)

don't you really want to get an absolute path here though?   the comment makes it sound like this isn't a complete solution.  should we be calling realpath() on the return value from fullPath()?  Do we even want to call fullPath(), since we're not using the stored cwd when we open the binary in LiveProcess::create()?

Joel Hestness Sept. 17, 2015, 12:47 p.m. (Sept. 17, 2015, 12:47 p.m.)

Sweet, yes! I thought there was a name for it other than "full" or "absolute" path, but had trouble finding it.

Change Summary:

Addressed Steve's notes.

Description:

~		Changeset 11092:03dde9f9798c
	~	Changeset 11092:fba06d1c46b0

		syscall_emul: Bandage readlink /proc/self/exe

		The recent changeset to readlink() to handle reading the /proc/self/exe link
		introduces a number of problems. This patch fixes two:

		1) Because readlink() called on /proc/self/exe now uses LiveProcess::progName()
		to find the binary path, it will only get the zeroth parameter of the simulated
		system command line. However, if a config script also specifies the process'
		executable, the executable parameter is used to create the LiveProcess rather
		than the zeroth command line parameter. Thus, the zeroth command line parameter
		is not necessarily the correct path to the binary executing in the simulated
		system. To fix this, add a LiveProcess data member, 'executable', which is
		correctly set during instantiation and returned from progName().

		2) If a config script allows a user to pass a relative path as the zeroth
		simulated system command line parameter or process executable, readlink() will
		incorrecly return a relative path when called on '/proc/self/exe'.
		/proc/self/exe is always set to a full path, so running benchmarks can fail if
		a relative path is returned. To fix this, clean up the handling of
		LiveProcess::progName() within readlink() to get the full binary path.

		NOTE: This patch still leaves the potential problem that host full path to the
		binary bleeds into the simulated system, potentially causing apparent
		non-deterministic simulated system execution.

Diff:

Revision 2 (+42 -19)

Show changes

	src/sim/process.hh
	src/sim/process.cc
	src/sim/syscall_emul.cc

Description:

~		Changeset 11092:fba06d1c46b0
	~	Changeset 11092:c0a2ea0a12b2

		syscall_emul: Bandage readlink /proc/self/exe

		The recent changeset to readlink() to handle reading the /proc/self/exe link
		introduces a number of problems. This patch fixes two:

		1) Because readlink() called on /proc/self/exe now uses LiveProcess::progName()
		to find the binary path, it will only get the zeroth parameter of the simulated
		system command line. However, if a config script also specifies the process'
		executable, the executable parameter is used to create the LiveProcess rather
		than the zeroth command line parameter. Thus, the zeroth command line parameter
		is not necessarily the correct path to the binary executing in the simulated
		system. To fix this, add a LiveProcess data member, 'executable', which is
		correctly set during instantiation and returned from progName().

		2) If a config script allows a user to pass a relative path as the zeroth
		simulated system command line parameter or process executable, readlink() will
		incorrecly return a relative path when called on '/proc/self/exe'.
		/proc/self/exe is always set to a full path, so running benchmarks can fail if
		a relative path is returned. To fix this, clean up the handling of
		LiveProcess::progName() within readlink() to get the full binary path.

		NOTE: This patch still leaves the potential problem that host full path to the
		binary bleeds into the simulated system, potentially causing apparent
		non-deterministic simulated system execution.

Diff:

Revision 3 (+43 -19)

Show changes

	src/sim/process.hh
	src/sim/process.cc
	src/sim/syscall_emul.cc

src/sim/syscall_emul.cc (Diff revision 3)

should just be 'readlink() should return the path of the executable'... the stuff about argv[0] is not relevant here

src/sim/syscall_emul.cc (Diff revision 3)

say 'absolute' rather than 'full'

src/sim/syscall_emul.cc (Diff revision 3)

trailing slash on /proc/self/exe/ brings up an interesting point: the test above only compares with the version w/o the trailing slash. at the very least the error message should be consistent...

src/sim/syscall_emul.cc (Diff revision 3)

should be free() rather than delete? might be safer just to allocate a local array of PATH_MAX on the stack

Change Summary:

Addressed Steve's comments

Description:

~		Changeset 11092:c0a2ea0a12b2
	~	Changeset 11092:3f4435aa225e

		syscall_emul: Bandage readlink /proc/self/exe

		The recent changeset to readlink() to handle reading the /proc/self/exe link
		introduces a number of problems. This patch fixes two:

		1) Because readlink() called on /proc/self/exe now uses LiveProcess::progName()
		to find the binary path, it will only get the zeroth parameter of the simulated
		system command line. However, if a config script also specifies the process'
		executable, the executable parameter is used to create the LiveProcess rather
		than the zeroth command line parameter. Thus, the zeroth command line parameter
		is not necessarily the correct path to the binary executing in the simulated
		system. To fix this, add a LiveProcess data member, 'executable', which is
		correctly set during instantiation and returned from progName().

		2) If a config script allows a user to pass a relative path as the zeroth
		simulated system command line parameter or process executable, readlink() will
		incorrecly return a relative path when called on '/proc/self/exe'.
		/proc/self/exe is always set to a full path, so running benchmarks can fail if
		a relative path is returned. To fix this, clean up the handling of
		LiveProcess::progName() within readlink() to get the full binary path.

		NOTE: This patch still leaves the potential problem that host full path to the
		binary bleeds into the simulated system, potentially causing apparent
		non-deterministic simulated system execution.

Diff:

Revision 4 (+40 -19)

Show changes

	src/sim/process.hh
	src/sim/process.cc
	src/sim/syscall_emul.cc

Ship It!

src/sim/syscall_emul.cc (Diff revision 4)

This comment is inaccurate, and therefore may lead to some confusion. There is no non-determinism, apparent or otherwise. I think 2) is sufficient to describe the problem.

If you do want to add a TODO it should probably simply say that this code should be revisited once an FS redirection scheme is in place, the lack of which is causing the real problem.

Show all issues

Steve Reinhardt Sept. 24, 2015, 12:19 p.m. (Sept. 24, 2015, 12:19 p.m.)

I agree with Tony---this is really just an example of host state leaking into the simulation, which happens in other ways in SE execution as well (often to do with the filesystem). As we mentioned on the list, we have some patches that implement a basic path redirection scheme that should give people the tools to make files appear to gem5 at the same path even if they're at different paths on the host. We hope to get those posted as soon as we get past some internal deadlines, probably in a few weeks.

Joel Hestness Sept. 24, 2015, 3:14 p.m. (Sept. 24, 2015, 3:14 p.m.)

I disagree with your assertion that this does not result in apparent non-determinism; I spent a couple hours tracking down why gem5-gpu regressions wouldn't pass immediately after I updated them. This most certainly appeared as non-determinic/non-repeatable simulation.

I think I'm ok with removing the TODO, but then I'd prefer that (2) be more precise than being classified as "unexpected behavior". Would you mind if I changed this to:

    //  2) The host's full path to the running benchmark changes from one
    //     simulation to another. This can result in different simulated
    //     performance since the simulated system will process the binary
    //     path differently, **even if the binary does not change**

Steve Reinhardt Sept. 24, 2015, 3:28 p.m. (Sept. 24, 2015, 3:28 p.m.)

That looks good to me. I guess "apparent" is in the eye of the beholder :).

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Description:

Changeset 11092:03dde9f9798c

Changeset 11092:fba06d1c46b0

Diff:

Description:

Changeset 11092:fba06d1c46b0

Changeset 11092:c0a2ea0a12b2

Diff:

Change Summary:

Description:

Changeset 11092:c0a2ea0a12b2

Changeset 11092:3f4435aa225e

Diff:

Status: Closed (submitted)