Harden yt-dlp binary resolution and analysis subprocess handling
- get_yt_dlp_path() no longer implicitly executes a bare "yt-dlp" from PATH (on Windows that lookup includes the CWD, so a planted binary in a writable directory could be run). A system yt-dlp is used only behind the explicit advanced.allow_system_ytdlp config opt-in, and then always as a which()-resolved absolute path. Analysis and download refuse to exec the not-installed sentinel. - Analysis subprocesses now run in their own session and the whole process group is killed on timeout, so deno grandchildren no longer leak; partial stderr is preserved and logged, and output decoding is pinned to utf-8 with replacement (Windows locale codecs crashed on non-UTF8 titles). - Flat-playlist entries are filtered for None (private/deleted first video no longer breaks analysis). - update_yt_dlp() normalizes the sentinel to Path, unbreaking the pip fallback path that crashed on str.exists(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,7 +1,10 @@
|
||||
from typing import TYPE_CHECKING, Any, Dict, List, Optional, cast
|
||||
|
||||
import json
|
||||
import os
|
||||
import signal
|
||||
import subprocess
|
||||
import sys
|
||||
from PySide6.QtCore import QMetaObject, Qt, Q_ARG, QThread, Signal, QTimer
|
||||
from PySide6.QtWidgets import QMessageBox
|
||||
|
||||
@@ -95,13 +98,56 @@ class AnalysisThread(QThread):
|
||||
if self.geo_proxy_url:
|
||||
cmd.extend(["--geo-verification-proxy", self.geo_proxy_url])
|
||||
|
||||
@staticmethod
|
||||
def _run_ytdlp(cmd: list, timeout: int) -> subprocess.CompletedProcess:
|
||||
"""Run yt-dlp capturing output, killing the whole process group on timeout.
|
||||
|
||||
subprocess.run() only kills the direct child on TimeoutExpired, leaking
|
||||
grandchildren (deno); it also loses any stderr produced before the
|
||||
timeout. Use Popen with a new session so the entire group can be
|
||||
reaped, and surface the partial stderr in the raised exception.
|
||||
"""
|
||||
popen_kwargs: Dict[str, Any] = {
|
||||
"stdout": subprocess.PIPE,
|
||||
"stderr": subprocess.PIPE,
|
||||
"text": True,
|
||||
"encoding": "utf-8",
|
||||
"errors": "replace",
|
||||
}
|
||||
if sys.platform == "win32":
|
||||
popen_kwargs["creationflags"] = SUBPROCESS_CREATIONFLAGS
|
||||
else:
|
||||
popen_kwargs["start_new_session"] = True
|
||||
|
||||
proc = subprocess.Popen(cmd, **popen_kwargs)
|
||||
try:
|
||||
stdout, stderr = proc.communicate(timeout=timeout)
|
||||
except subprocess.TimeoutExpired as exc:
|
||||
if sys.platform == "win32":
|
||||
subprocess.run(
|
||||
["taskkill", "/F", "/T", "/PID", str(proc.pid)],
|
||||
stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
|
||||
creationflags=SUBPROCESS_CREATIONFLAGS,
|
||||
)
|
||||
else:
|
||||
try:
|
||||
os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
|
||||
except (ProcessLookupError, PermissionError, OSError):
|
||||
pass
|
||||
stdout, stderr = proc.communicate()
|
||||
exc.stdout, exc.stderr = stdout, stderr
|
||||
raise
|
||||
return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr)
|
||||
|
||||
def _analyze_url_with_subprocess(self, url: str) -> None:
|
||||
"""Analyze URL using yt-dlp executable."""
|
||||
if self._cancelled:
|
||||
return
|
||||
|
||||
yt_dlp_path = get_yt_dlp_path()
|
||||
if not yt_dlp_path:
|
||||
if not yt_dlp_path or str(yt_dlp_path) == "yt-dlp":
|
||||
# Sentinel: no managed binary and no opted-in system binary.
|
||||
# Never exec a bare command name from PATH.
|
||||
logger.error("yt-dlp executable not found. Please install yt-dlp first.")
|
||||
self.analysis_error.emit(_("errors.ytdlp_not_found"))
|
||||
self.playlist_info_visible.emit(False)
|
||||
@@ -118,12 +164,10 @@ class AnalysisThread(QThread):
|
||||
logger.debug(f"Executing yt-dlp command: {cmd}")
|
||||
|
||||
try:
|
||||
result = subprocess.run(
|
||||
cmd, capture_output=True, text=True, timeout=300,
|
||||
creationflags=SUBPROCESS_CREATIONFLAGS
|
||||
)
|
||||
except subprocess.TimeoutExpired:
|
||||
logger.error("Analysis timed out")
|
||||
result = self._run_ytdlp(cmd, timeout=300)
|
||||
except subprocess.TimeoutExpired as e:
|
||||
stderr_tail = (e.stderr or "")[-500:] if isinstance(e.stderr, str) else ""
|
||||
logger.error(f"Analysis timed out. Partial stderr: {stderr_tail}")
|
||||
self.analysis_error.emit(_("errors.timeout"))
|
||||
return
|
||||
|
||||
@@ -181,7 +225,8 @@ class AnalysisThread(QThread):
|
||||
if first_info.get("_type") == "playlist":
|
||||
result_data["is_playlist"] = True
|
||||
result_data["playlist_info"] = first_info
|
||||
playlist_entries = first_info.get("entries", [])
|
||||
# Private/deleted videos can appear as None entries in flat playlists
|
||||
playlist_entries = [e for e in first_info.get("entries", []) if e]
|
||||
result_data["playlist_entries"] = playlist_entries
|
||||
|
||||
if not playlist_entries:
|
||||
@@ -201,15 +246,12 @@ class AnalysisThread(QThread):
|
||||
self._add_auth_options(cmd_single)
|
||||
|
||||
try:
|
||||
result_single = subprocess.run(
|
||||
cmd_single, capture_output=True, text=True, timeout=60,
|
||||
creationflags=SUBPROCESS_CREATIONFLAGS
|
||||
)
|
||||
result_single = self._run_ytdlp(cmd_single, timeout=60)
|
||||
if result_single.returncode == 0:
|
||||
result_data["video_info"] = json.loads(result_single.stdout)
|
||||
else:
|
||||
result_data["video_info"] = first_video_entry
|
||||
except subprocess.TimeoutExpired:
|
||||
except (subprocess.TimeoutExpired, json.JSONDecodeError):
|
||||
result_data["video_info"] = first_video_entry
|
||||
|
||||
if self._cancelled:
|
||||
|
||||
Reference in New Issue
Block a user