Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions gprofiler/profilers/perf.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from collections import Counter, defaultdict
from pathlib import Path
from subprocess import Popen
from tempfile import NamedTemporaryFile
from threading import Event
from typing import Any, Dict, Iterable, List, Optional

Expand Down Expand Up @@ -322,27 +323,26 @@ def wait_and_script(self) -> str:
# (unlike Popen.communicate())
assert self._process is not None and self._process.stderr is not None
logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore

try:
inject_data = Path(f"{str(perf_data)}.inject")
if self._inject_jit:
run_process(
[perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)],
with NamedTemporaryFile(dir=os.path.dirname(self._output_path), suffix=".inject") as inject_data:
if self._inject_jit:
run_process(
[perf_path(), "inject", "--jit", "-o", str(inject_data), "-i", str(perf_data)],
)
perf_data = Path(inject_data.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a problematic flow:

  1. perf_data = temporary file
  2. Exception is raised during perf script
  3. finally runs and tries to unlink perf_data which is already unlinked.

I suggest you use a 3rd variable (like perf_script_input), then set it to either perf_data OR inject_data, and only unlink perf_data as inject_data is automatically removed.


perf_script_proc = run_process(
[perf_path(), "script", "-F", "+pid", "-i", str(perf_data)],
suppress_log=True,
)
perf_data.unlink()
perf_data = inject_data

perf_script_proc = run_process(
[perf_path(), "script", "-F", "+pid", "-i", str(perf_data)],
suppress_log=True,
)
return perf_script_proc.stdout.decode("utf8")
return perf_script_proc.stdout.decode("utf8")
finally:
perf_data.unlink()
if self._inject_jit:
# might be missing if it's already removed.
# might be existing if "perf inject" itself fails
remove_path(inject_data, missing_ok=True)
# always read its stderr
# using read1() which performs just a single read() call and doesn't read until EOF
# (unlike Popen.communicate())
assert self._process is not None and self._process.stderr is not None
logger.debug(f"{self._log_name} run output", perf_stderr=self._process.stderr.read1()) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these related?



@register_profiler(
Expand Down
19 changes: 8 additions & 11 deletions gprofiler/utils/fs.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
import errno
import os
import shutil
from pathlib import Path
from secrets import token_hex
from tempfile import NamedTemporaryFile

from gprofiler.platform import is_windows
from gprofiler.utils import remove_path, run_process
from gprofiler.utils import run_process


def safe_copy(src: str, dst: str) -> None:
Expand All @@ -27,29 +26,27 @@ def is_rw_exec_dir(path: str) -> bool:
"""
Is 'path' rw and exec?
"""
# randomize the name - this function runs concurrently on paths of in same mnt namespace.
test_script = Path(path) / f"t-{token_hex(10)}.sh"

# try creating & writing
try:
os.makedirs(path, 0o755, exist_ok=True)
test_script.write_text("#!/bin/sh\nexit 0")
test_script.chmod(0o755)
test_script = NamedTemporaryFile(dir=path, suffix=".sh")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that to have NamedTemporaryFile actually remove the file, you need __exit__ to be called. So the usage should be:

with NamedTemporaryFile(...) as test_script:
    do function logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a simple test to check it and it gets closed everytime, even with "sys.exit(1) between these lines:

tf = tempfile.NamedTemporaryFile(dir="/home/kitter/Documents/june-gprof/my-gprof-fork/gprofiler", suffix=".tester")

with open(tf.name, "w") as f:
    f.write("my-tester")
print(Path(tf.name).read_text())
time.sleep(5)

but I changed it accordingly anyway :)

with open(test_script.name, "w") as f:
f.write("#!/bin/sh\nexit 0")
os.chmod(test_script.name, 0o755)
test_script.file.close()
except OSError as e:
if e.errno == errno.EROFS:
# ro
return False
remove_path(test_script)
raise

# try executing
try:
run_process([str(test_script)], suppress_log=True)
run_process([str(test_script.name)], suppress_log=True)
except PermissionError:
# noexec
return False
finally:
test_script.unlink()

return True

Expand Down