exec -internal-pid-file cannot be used reliably

[stepancheg]

Description

runsc exec has -internal-pid-file flag.

According to this code, this file may not be written atomically:

gvisor/runsc/cmd/exec.go

Lines 291 to 296 in 6308502

	// File appeared, check whether pid is fully written.
	pid, err := strconv.Atoi(string(pidb))
	if err != nil {
	return false, nil
	}
	return pid == cmd.Process.Pid, nil

(I think it is generally written atomically, and I struggle to find examples when it is not, but Linux does not provide such guarantees, and gVisor itself does not assume it is written atomically).

So practically, when using runsc exec without -detach flag (that executes code above), it is not possible to reliably know process id when using -internal-pid-file.

Is this feature related to a specific bug?

No response

Do you have a specific solution in mind?

I have two options in mind

• write trailing newline to -internal-pid-file file, so a user can assume that the file is fully written when trailing newline is present. That looks like the best option, if backwards compatibility is not an issue
• add a new flag like -exec-info-file that would contain a JSON with process ids. Overengineering for such simple problem
• do nothing because the problem is improbable

[EtiennePerot]

Adding a newline sounds OK to me.

[avagin]

docker exec fails with this change:

$ docker run --runtime runsc -d --rm --name test alpine sleep 1000
93a0aaec5399a673680e71c1fdd36c55590c6ab1247c93198c04c5bf549e0e8a
$ docker exec test echo hello
hello
failed to retrieve OCI runtime exec pi: strconv.Atoi: parsing "1021797\n": invalid syntaxd: unknown

[EtiennePerot]

Darn.

I suggest writing the PID (without \n) to a temporary file next to the intended PID file, then rename the file to move it into place. That would also ensure it is fully written if it exists.