fix(downloads): stop a ReDoS title and a fatal thumbnail convert from wedging/killing downloads
Three prod-observed download bugs, root-caused from a frozen prod queue with the
worker pinned at 100% CPU:
- ReDoS in normalize_title: _TRAILING_HASHTAGS had two overlapping unbounded
quantifiers around the required letter, so a Facebook title with a long hashtag
cluster that is NOT at the end ("... #a #b #c | Page") backtracked catastrophically.
It runs inside the yt-dlp progress hook holding the GIL, so one such download froze
the whole worker (and could stall feed enrichment too - sync/videos.py calls it).
Rewrite with a zero-width "has a letter" lookahead + a possessive tag body: one
unambiguous split, nothing to backtrack. 40-tag stress case: ~0.1ms.
- Reddit download killed by a cosmetic thumbnail convert: yt-dlp's
FFmpegThumbnailsConvertor runs "-bsf:v mjpeg2jpeg" on a JPEG-served-under-a-.png-name
(Reddit) and fails; being a postprocessor, it failed the WHOLE download even though
the media was fine. Drop it and produce the <base>.jpg poster/og:image ourselves via
editmod.thumbnail_to_jpg (plain ffmpeg decode->mjpeg, best-effort -> ensure_poster
cuts a frame on failure). The FB Messenger / MS Teams share preview keeps working
(posters verified as real JPEGs).
- classify: map "authentication is required" (Reddit's auth wall) to login_required
instead of unknown, so the UI explains it rather than offering a pointless retry.
Tests: ReDoS-timing + numeric-marker regression, the Reddit classify case, and
thumbnail_to_jpg (real JPEG out / None on bad input, never raises).
This commit is contained in:
@@ -489,6 +489,28 @@ def build_poster_cmd(src: Path, dest: Path, at_s: float) -> list[str]:
|
||||
]
|
||||
|
||||
|
||||
def thumbnail_to_jpg(src: Path | None, dest: Path, timeout_s: int = 30) -> Path | None:
|
||||
"""Convert a downloaded thumbnail (webp / png / jpeg — whatever the source shipped) into a real
|
||||
baseline JPEG for the `<name>.jpg` poster + og:image share sidecar.
|
||||
|
||||
Uses a plain ffmpeg decode→mjpeg, deliberately NOT yt-dlp's `FFmpegThumbnailsConvertor` (whose
|
||||
`-bsf:v mjpeg2jpeg` path crashes on some already-JPEG thumbnails, e.g. Reddit's, and used to
|
||||
fail the entire download). Best-effort: returns None on any failure, so the caller falls back to
|
||||
a cut video frame via `ensure_poster` and the download still succeeds."""
|
||||
if src is None or not src.exists():
|
||||
return None
|
||||
try:
|
||||
subprocess.run(
|
||||
["ffmpeg", "-y", "-hide_banner", "-loglevel", "error", "-nostdin",
|
||||
"-i", str(src), "-frames:v", "1", "-update", "1", str(dest)],
|
||||
capture_output=True, timeout=timeout_s, check=True,
|
||||
)
|
||||
except (subprocess.TimeoutExpired, subprocess.CalledProcessError, OSError) as exc:
|
||||
log.info("thumbnail->jpg conversion failed (%s); poster falls back to a cut frame", str(exc)[:120])
|
||||
return None
|
||||
return dest if dest.exists() else None
|
||||
|
||||
|
||||
def ensure_poster(asset, download_root: str, timeout_s: int = 30) -> str | None:
|
||||
"""Generate a poster frame for a ready asset that has no thumbnail (e.g. a direct media URL
|
||||
like a reddit HLS manifest, which yt-dlp can't get a thumbnail for). Written as the media's
|
||||
|
||||
@@ -21,6 +21,10 @@ _RULES: tuple[tuple[str, str], ...] = (
|
||||
("available to this channel's members", "login_required"),
|
||||
("join this channel", "login_required"),
|
||||
("this video is only available to", "login_required"),
|
||||
# Some extractors (e.g. Reddit) gate content behind a logged-in account: "Account
|
||||
# authentication is required." — an auth wall we deliberately don't cross, not a transient error.
|
||||
("authentication is required", "login_required"),
|
||||
("account authentication", "login_required"),
|
||||
# Geo-restricted.
|
||||
("in your country", "geo_blocked"),
|
||||
("available in your location", "geo_blocked"),
|
||||
|
||||
@@ -211,9 +211,13 @@ def build_ydl_opts(
|
||||
opts["postprocessors"].append(
|
||||
{"key": "FFmpegMetadata", "add_chapters": True, "add_metadata": True}
|
||||
)
|
||||
# Normalize the written thumbnail to jpg so we always have a `<name>.jpg` poster sidecar.
|
||||
# `already_have_thumbnail` keeps that file on disk after embedding (default deletes it).
|
||||
opts["postprocessors"].append({"key": "FFmpegThumbnailsConvertor", "format": "jpg"})
|
||||
# NOTE: we deliberately do NOT add yt-dlp's `FFmpegThumbnailsConvertor` here. Its jpg conversion
|
||||
# runs `ffmpeg -bsf:v mjpeg2jpeg` when the written thumbnail is already JPEG bytes (Reddit serves
|
||||
# a JPEG under a `.png` name), and that bitstream filter *fails* on some such images — and since a
|
||||
# postprocessor error is fatal, it used to kill the WHOLE download even though the media was fine.
|
||||
# The `<name>.jpg` poster sidecar is instead produced robustly by our own pipeline (worker
|
||||
# `_download` → `editmod.thumbnail_to_jpg`, with `ensure_poster` cutting a video frame as a
|
||||
# fallback), so the og:image share preview (Facebook Messenger / MS Teams) keeps working.
|
||||
if s["embed_thumbnail"]:
|
||||
opts["postprocessors"].append(
|
||||
{"key": "EmbedThumbnail", "already_have_thumbnail": True}
|
||||
|
||||
@@ -34,7 +34,15 @@ _ENGAGEMENT_PREFIX = re.compile(
|
||||
re.IGNORECASE,
|
||||
)
|
||||
_LETTER_RUN = re.compile(r"[^\W\d_]+", re.UNICODE)
|
||||
_TRAILING_HASHTAGS = re.compile(r"(?:\s+#[^\s#]*[^\W\d\s_][^\s#]*)+\s*$", re.UNICODE)
|
||||
# Trailing SEO hashtag cluster: a run of whitespace-separated `#tags` at the very end, where each
|
||||
# tag contains at least one letter (a bare numeric "#3" episode marker is NOT a hashtag and
|
||||
# survives). The "has a letter" test is a zero-width lookahead and the tag body is matched
|
||||
# possessively (`*+`), so there is exactly ONE way to split each tag and nothing to backtrack over.
|
||||
# The previous form `[^\s#]*[letter][^\s#]*` had TWO overlapping unbounded quantifiers around the
|
||||
# required letter: on a long hashtag run that ends with trailing non-tag text (a Facebook
|
||||
# "… #a #b #c | Page" title), the `$` anchor fails and the engine backtracks catastrophically
|
||||
# (ReDoS) — it pinned a worker thread at 100% CPU forever inside the download progress hook.
|
||||
_TRAILING_HASHTAGS = re.compile(r"(?:\s+#(?=[^\s#]*[^\W\d\s_])[^\s#]*+)+\s*$", re.UNICODE)
|
||||
_REPEAT_PUNCT = re.compile(r"([!?.,\-])\1{1,}")
|
||||
|
||||
# Short function words → lowercased when the title is de-shouted (proper title-case style).
|
||||
|
||||
@@ -617,8 +617,12 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe
|
||||
layout, retention_days = _download_settings()
|
||||
rel = storage.rel_path(meta, ext, layout, asset_id=asset_id)
|
||||
thumb = _find_thumbnail(staging, produced)
|
||||
# Convert the raw thumbnail (webp/png/jpeg) to a real JPEG ourselves — robustly — for the
|
||||
# poster + og:image sidecar, now that yt-dlp's fatal thumbnail convertor is gone. On failure
|
||||
# this is None and write_sidecars writes no poster, so ensure_poster cuts a frame instead.
|
||||
poster_jpg = editmod.thumbnail_to_jpg(thumb, staging / "poster.jpg")
|
||||
storage.place_file(produced, settings.download_root, rel)
|
||||
nfo_ok = storage.write_sidecars(settings.download_root, rel, meta, thumb)
|
||||
nfo_ok = storage.write_sidecars(settings.download_root, rel, meta, poster_jpg)
|
||||
|
||||
final = storage.abs_path(settings.download_root, rel)
|
||||
size = final.stat().st_size if final.exists() else None
|
||||
|
||||
@@ -12,6 +12,7 @@ def test_real_ytdlp_messages_map_to_categories():
|
||||
"ERROR: [youtube] abc: Sign in to confirm your age. This video may be inappropriate": "login_required",
|
||||
"ERROR: [youtube] abc: Sign in to confirm you're not a bot": "login_required",
|
||||
"ERROR: [youtube] abc: Join this channel to get access to members-only content": "login_required",
|
||||
"ERROR: [Reddit] 1vi1x56: Account authentication is required.": "login_required",
|
||||
"ERROR: [youtube] abc: The uploader has not made this video available in your country": "geo_blocked",
|
||||
"ERROR: [youtube] abc: Requested format is not available": "format_unavailable",
|
||||
"ERROR: [youtube] abc: Only images are available for download": "format_unavailable",
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
"""`thumbnail_to_jpg`: robustly turn a downloaded thumbnail into a real JPEG poster/og:image.
|
||||
|
||||
This replaced yt-dlp's `FFmpegThumbnailsConvertor`, whose `-bsf:v mjpeg2jpeg` path crashed on some
|
||||
already-JPEG thumbnails (Reddit serves a JPEG under a `.png` name) — and because a postprocessor
|
||||
error is fatal, it used to fail the WHOLE download. The key regression property here is that a bad
|
||||
thumbnail returns None (poster falls back to a cut frame) instead of raising.
|
||||
"""
|
||||
import shutil
|
||||
import subprocess
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from app.downloads import edit
|
||||
|
||||
_ffmpeg_missing = shutil.which("ffmpeg") is None
|
||||
needs_ffmpeg = pytest.mark.skipif(_ffmpeg_missing, reason="ffmpeg not available")
|
||||
|
||||
|
||||
def _is_jpeg(p: Path) -> bool:
|
||||
return p.exists() and p.read_bytes()[:3] == b"\xff\xd8\xff"
|
||||
|
||||
|
||||
@needs_ffmpeg
|
||||
def test_converts_a_png_to_a_real_jpeg(tmp_path):
|
||||
src = tmp_path / "thumb.png"
|
||||
subprocess.run(
|
||||
["ffmpeg", "-y", "-loglevel", "error", "-f", "lavfi",
|
||||
"-i", "color=c=red:s=64x64", "-frames:v", "1", str(src)],
|
||||
check=True,
|
||||
)
|
||||
out = edit.thumbnail_to_jpg(src, tmp_path / "poster.jpg")
|
||||
assert out is not None and _is_jpeg(out)
|
||||
|
||||
|
||||
def test_returns_none_for_a_missing_source(tmp_path):
|
||||
assert edit.thumbnail_to_jpg(None, tmp_path / "p.jpg") is None
|
||||
assert edit.thumbnail_to_jpg(tmp_path / "nope.png", tmp_path / "p.jpg") is None
|
||||
|
||||
|
||||
@needs_ffmpeg
|
||||
def test_returns_none_on_garbage_input_instead_of_raising(tmp_path):
|
||||
# The whole point: a thumbnail ffmpeg can't decode must NOT crash the download.
|
||||
bad = tmp_path / "bad.png"
|
||||
bad.write_bytes(b"this is definitely not an image")
|
||||
assert edit.thumbnail_to_jpg(bad, tmp_path / "p.jpg") is None
|
||||
@@ -24,6 +24,29 @@ def test_trailing_hashtags_are_stripped():
|
||||
assert normalize_title("Great cooking video #food #yummy") == "Great cooking video"
|
||||
|
||||
|
||||
def test_numeric_episode_marker_is_not_treated_as_a_hashtag():
|
||||
# A bare "#3" is an episode marker, not an SEO hashtag (no letters) → it must survive.
|
||||
assert normalize_title("My show #3") == "My show #3"
|
||||
|
||||
|
||||
def test_midstring_hashtags_are_fast_and_kept_redos_regression():
|
||||
# Facebook prepends an engagement prefix and appends a page name, leaving a long hashtag cluster
|
||||
# in the MIDDLE. That is not a trailing SEO cluster, so the tags stay — and, critically, this
|
||||
# exact shape used to trigger catastrophic backtracking (ReDoS) in `_TRAILING_HASHTAGS` that
|
||||
# pinned a download-worker thread at 100% CPU forever inside the yt-dlp progress hook.
|
||||
import time
|
||||
|
||||
raw = (
|
||||
"398K views · 6.1K reactions | The money should follow 🙄 #reel #reels #meme #memes "
|
||||
"#fun #funny #work #worklife #office #viral #trending #fyp #foryou #foryoupage | Memebonanza4U"
|
||||
)
|
||||
t = time.perf_counter()
|
||||
out = normalize_title(raw) or ""
|
||||
assert time.perf_counter() - t < 1.0 # linear now; the old regex ran for minutes / never returned
|
||||
assert "#foryoupage" in out and "#reel" in out # mid-string hashtags preserved (not trailing)
|
||||
assert "views" not in out # the engagement-count prefix was still stripped
|
||||
|
||||
|
||||
def test_emoji_and_symbols_are_dropped():
|
||||
assert "🔥" not in (normalize_title("Fire recipe 🔥🔥") or "")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user