Check if PID file is empty before reading #1458
Conversation
aswatt
requested review from
abenbachir,
Dmitry-Matveev,
hestolz and
simathih
|
Can approve for sure but do we have any scenario where this would occur? also this method (checking |
| @@ -369,7 +369,7 @@ def stop_telemetry_process(): | |||
| for pid in f.readlines(): | |||
| # Verify the pid actually belongs to omsagent. | |||
| cmd_file = os.path.join("/proc", str(pid.strip("\n")), "cmdline") | |||
| if os.path.exists(cmd_file): | |||
| if os.path.exists(cmd_file) and (os.path.getsize(cmd_file) > 0): | |||
There was a problem hiding this comment.
This is a time of check to time of use error waiting to happen.
Between this line and the next the PID may be gone, and the open below will fail. Instead the code should attempt to open the file unconditionally, and have a try/catch surrounding it for failure due to the PID not existing.
That solves one issue...
| @@ -369,7 +369,7 @@ def stop_telemetry_process(): | |||
| for pid in f.readlines(): | |||
| # Verify the pid actually belongs to omsagent. | |||
| cmd_file = os.path.join("/proc", str(pid.strip("\n")), "cmdline") | |||
| if os.path.exists(cmd_file): | |||
| if os.path.exists(cmd_file) and (os.path.getsize(cmd_file) > 0): | |||
| with open(cmd_file, "r") as pidf: | |||
| cmdline = pidf.readlines() | |||
| if cmdline[0].find("omsagent.py") >= 0 and cmdline[0].find("-telemetry") >= 0: | |||
There was a problem hiding this comment.
Here we start our next time of check to time of use issue...
We validate here that the command line includes omsagent.py and -telemetry...
then on line 377 we send the kill command.
Between the time opening the file, checking that it belongs to omsagent.py the omsagent could have already been stopped, and the PID re-used by another process. The kill command will at that point send a signal to an innocent process.
Instead this should likely be managed using systemd or some other service manager that can keep track of PID's using cgroups.
If the PID cmdline file exists but is empty, then readlines() returns an empty list
[], which will cause thecmdline[0]checks to fail saying "list index out of range". This verifies that the file both exists and has contents when doing this check, which should cover this edge case.