Skip to content

security: Add input validation for file path in FFmpegVideoSource constructor #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Unluckyathecking wants to merge 2 commits into
base: master
Choose a base branch
from
Open

security: Add input validation for file path in FFmpegVideoSource constructor #3

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c41a0f
security: Add input validation for file path in FFmpegVideoSource con…
github-actions[bot] May 25, 2026
82ff28e
ci: jumpstart kimi review (post Pro upgrade)
Unluckyathecking May 26, 2026
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
14 changes: 14 additions & 0 deletions src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java
View file
Open in desktop

@devin-ai-integration devin-ai-integration Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 Pre-existing: getFrame() accesses shared state without holding any lock

In the pre-existing getFrame() method, the read lock is released after validateOpen() at line 176, but subsequent operations — bounds checking via metadata.frameCount() (line 180), cache access (line 186), and frame decoding via decodeFrameAt() (lines 192-195, which accesses grabber and converter) — all happen without any lock. If close() is called concurrently, metadata could be reset, grabber/converter set to null, etc. This is a pre-existing thread-safety gap not introduced by this PR, but worth noting since the PR touches the same class.

(Refers to lines 171-197)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.opensourcephysics.cabrillo.tracker.video;

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [suggestion] PR title incorrectly states constructor instead of open method

The PR title says "Add input validation for file path in FFmpegVideoSource constructor" but the diff modifies the open(Path file) method, not any constructor. The constructors at lines 58 and 67 take cacheSize, not a file path. This indicates the planner or worker did not accurately read the code structure.


import java.awt.image.BufferedImage;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.concurrent.locks.ReentrantReadWriteLock;

Expand Down Expand Up @@ -67,8 +68,21 @@

@Override
public void open(Path file) {
lock.writeLock().lock();

Check warning on line 71 in src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Blocking filesystem I/O held inside write lock 

Lines 71-77 perform `Files.exists()` and `Files.isReadable()` calls while holding `lock.writeLock()`. These are blocking filesystem operations that can stall for extended periods (network filesystems, slow disks). Holding a write lock during I/O prevents all concurrent readers from accessing metadata queries like `getFrameRate()`, `getWidth()`, `getHeight()` — all of which only need `lock.readLock()`. The validation should occur **before** acquiring the write lock, or the lock should be acquired only after validation succeeds.
Raw output 
    public void open(Path file) {
        if (file == null) {
            throw new IllegalArgumentException("Video file path cannot be null");
        }
        String pathStr = file.toString().trim();
        if (pathStr.isEmpty()) {
            throw new IllegalArgumentException("Video file path cannot be empty");
        }
        if (!Files.exists(file)) {
            throw new IllegalArgumentException("Video file does not exist: " + file);
        }
        if (!Files.isReadable(file)) {
            throw new IllegalArgumentException("Video file is not readable: " + file);
        }

        lock.writeLock().lock();
        try {
            if (isOpen()) {
                close();
            }

Check warning on line 71 in src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] TOCTOU race condition between exists() and isReadable() checks 

Line 71 checks `Files.exists(file)` and line 74 checks `Files.isReadable(file)`, but the file system state can change between these two calls (Time-Of-Check to Time-Of-Use). A file can be deleted or have permissions changed after `exists()` returns true but before `isReadable()` is called. The `isReadable()` call will simply return `false` in this case, but the pattern encourages a false sense of security. More critically, the `grabber.start()` call on line 88 will fail anyway with its own exception; these pre-checks are optimistic and don't eliminate the need for proper exception handling around `grabber.start()`.

@github-actions github-actions Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Blocking filesystem I/O held inside write lock

Lines 71-77 perform Files.exists() and Files.isReadable() calls while holding lock.writeLock(). These are blocking filesystem operations that can stall for extended periods (network filesystems, slow disks). Holding a write lock during I/O prevents all concurrent readers from accessing metadata queries like getFrameRate(), getWidth(), getHeight() — all of which only need lock.readLock(). The validation should occur before acquiring the write lock, or the lock should be acquired only after validation succeeds.

Suggested fix:

Suggested change
lock.writeLock().lock();
public void open(Path file) {
if (file == null) {
throw new IllegalArgumentException("Video file path cannot be null");
}
String pathStr = file.toString().trim();
if (pathStr.isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
if (!Files.exists(file)) {
throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (!Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);
}
lock.writeLock().lock();
try {
if (isOpen()) {
close();
}

@github-actions github-actions Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] TOCTOU race condition between exists() and isReadable() checks

Line 71 checks Files.exists(file) and line 74 checks Files.isReadable(file), but the file system state can change between these two calls (Time-Of-Check to Time-Of-Use). A file can be deleted or have permissions changed after exists() returns true but before isReadable() is called. The isReadable() call will simply return false in this case, but the pattern encourages a false sense of security. More critically, the grabber.start() call on line 88 will fail anyway with its own exception; these pre-checks are optimistic and don't eliminate the need for proper exception handling around grabber.start().

try {
if (file == null) {

Check failure on line 73 in src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[critical] Validation order bug: empty path check unreachable due to prior existence check 

On line 73, `file.toString().trim().isEmpty()` is checked **after** line 71's `!Files.exists(file)` check. For an empty path string like `Path.of("")`, `Files.exists()` returns `false`, so line 71 throws "Video file does not exist" **before** the empty-path check on line 73 ever executes. The empty path error message is dead code. The correct order must validate null → empty string → existence → readability.
Raw output 
            if (file == null) {
                throw new IllegalArgumentException("Video file path cannot be null");
            }
            String pathStr = file.toString().trim();
            if (pathStr.isEmpty()) {
                throw new IllegalArgumentException("Video file path cannot be empty");
            }
            if (!Files.exists(file)) {
                throw new IllegalArgumentException("Video file does not exist: " + file);
            }
            if (!Files.isReadable(file)) {
                throw new IllegalArgumentException("Video file is not readable: " + file);
            }

Check warning on line 73 in src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Path.toString().trim().isEmpty() is not a valid empty-path test for Path objects 

Line 73 uses `file.toString().trim().isEmpty()` to detect empty paths. However, `Path.of("")` produces a `Path` with an empty name; `Path.of("  ")` (spaces) produces a valid relative path that may or may not exist. The `trim()` suggests string-level thinking applied to `Path`. The proper check for a path with no name components is `file.getFileName() == null || file.getFileName().toString().isEmpty()`. The current check coincidentally works for `Path.of("")` because its string representation is `""`, but it's semantically incorrect and misleading.
Raw output 
            if (file.getFileName() == null || file.getFileName().toString().trim().isEmpty()) {
                throw new IllegalArgumentException("Video file path cannot be empty");
            }

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Missing validation for non-video file extensions

The planner rationale explicitly mentions "point to non-video files, potentially causing native crashes in FFmpeg." However, the implementation does not validate that the file has a video extension (e.g., .mp4, .avi, .mov). FFmpeg's avformat_open_input will attempt to probe any file, including binary or malicious files, which could trigger native crashes or hangs on malformed input. At minimum, a whitelist of extensions should be checked before passing to FFmpeg, or the caller should be documented as responsible for this validation.

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] TOCTOU race between Files.exists and Files.isReadable

Lines 73 and 76 check Files.exists(file) and Files.isReadable(file) in separate calls. Between these two calls, the file could be deleted or have its permissions changed. The correct pattern is to attempt the operation and handle the resulting IOException, or use Files.isReadable alone (which returns false for non-existent files per JavaDoc, though this is implementation-dependent). The current two-step check is a classic Time-of-Check Time-of-Use (TOCTOU) pattern.

Suggested fix:

Suggested change
if (file == null) {
if (!Files.isReadable(file)) {
throw new IllegalArgumentException("Video file does not exist or is not readable: " + file);
}

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] file.toString() may produce platform-specific string before trim check

On line 73, file.toString().trim().isEmpty() converts the Path to a string using the default filesystem's separator. For a Path created via Path.of(""), toString() returns "" on Unix but may return "." or other values on Windows depending on the provider. The trim() check is insufficient for detecting empty paths across platforms. A more robust check is file.toString().trim().isEmpty() || file.getNameCount() == 0 or simply checking if the normalized path is empty.

Suggested fix:

Suggested change
if (file == null) {
String pathStr = file.toString().trim();
if (pathStr.isEmpty() || file.getNameCount() == 0) {
throw new IllegalArgumentException("Video file path cannot be empty");
}

@github-actions github-actions Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 [critical] Validation order bug: empty path check unreachable due to prior existence check

On line 73, file.toString().trim().isEmpty() is checked after line 71's !Files.exists(file) check. For an empty path string like Path.of(""), Files.exists() returns false, so line 71 throws "Video file does not exist" before the empty-path check on line 73 ever executes. The empty path error message is dead code. The correct order must validate null → empty string → existence → readability.

Suggested fix:

Suggested change
if (file == null) {
if (file == null) {
throw new IllegalArgumentException("Video file path cannot be null");
}
String pathStr = file.toString().trim();
if (pathStr.isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
if (!Files.exists(file)) {
throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (!Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);
}

@github-actions github-actions Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Path.toString().trim().isEmpty() is not a valid empty-path test for Path objects

Line 73 uses file.toString().trim().isEmpty() to detect empty paths. However, Path.of("") produces a Path with an empty name; Path.of(" ") (spaces) produces a valid relative path that may or may not exist. The trim() suggests string-level thinking applied to Path. The proper check for a path with no name components is file.getFileName() == null || file.getFileName().toString().isEmpty(). The current check coincidentally works for Path.of("") because its string representation is "", but it's semantically incorrect and misleading.

Suggested fix:

Suggested change
if (file == null) {
if (file.getFileName() == null || file.getFileName().toString().trim().isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}

throw new IllegalArgumentException("Video file path cannot be null");
}
if (!file.toString().trim().isEmpty() && !Files.exists(file)) {

Check warning on line 76 in src/main/java/org/opensourcephysics/cabrillo/tracker/video/FFmpegVideoSource.java

View workflow job for this annotation

GitHub Actions / Kimi Code Review

[warning] Wrong exception type for I/O permission failure 

Line 76 throws `IllegalArgumentException` for a file that exists but is not readable. This is an I/O permission/authorization problem, not an illegal argument. `IllegalArgumentException` indicates caller provided an invalid value; `SecurityException` or `java.io.IOException` (or `java.nio.file.AccessDeniedException`) correctly signals a permission denied condition. Using `IllegalArgumentException` forces callers to catch the wrong exception type for access control logic.
Raw output 
                throw new SecurityException("Video file is not readable: " + file);

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 [critical] Empty path check ordered after Files.exists() call risks InvalidPathException

On line 73, the condition !file.toString().trim().isEmpty() && !Files.exists(file) calls Files.exists(file) before the empty-path guard on line 79. On Windows, Path.of("") or an empty-string Path passed to Files.exists() can throw InvalidPathException at the Files.exists() call site, bypassing the later empty check entirely. The empty check must precede any Files.exists() or Files.isReadable() call. Additionally, the empty check on line 79 is redundant because the first condition already evaluates !file.toString().trim().isEmpty().

Mechanism: Files.exists() delegates to the filesystem provider, which may reject malformed/empty paths before returning false. The catch block on line 89 only catches Exception from grabber.start(), not from the validation phase.

Fix: Reorder validations: null check → empty check → exists check → readable check. Remove the redundant empty check.

Suggested fix:

Suggested change
if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
if (file == null) {
throw new IllegalArgumentException("Video file path cannot be null");
}
String pathStr = file.toString().trim();
if (pathStr.isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
if (!Files.exists(file)) {
throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (!Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);
}

@github-actions github-actions Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] Wrong exception type for I/O permission failure

Line 76 throws IllegalArgumentException for a file that exists but is not readable. This is an I/O permission/authorization problem, not an illegal argument. IllegalArgumentException indicates caller provided an invalid value; SecurityException or java.io.IOException (or java.nio.file.AccessDeniedException) correctly signals a permission denied condition. Using IllegalArgumentException forces callers to catch the wrong exception type for access control logic.

Suggested fix:

Suggested change
if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
throw new SecurityException("Video file is not readable: " + file);

throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (Files.exists(file) && !Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);

@github-actions github-actions Bot May 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [warning] isOpen() called under write lock may deadlock or behave unexpectedly

On line 80, isOpen() acquires a readLock() while the current thread already holds the writeLock(). ReentrantReadWriteLock does NOT support lock downgrading from write to read when using the non-fair constructor (default). While ReentrantReadWriteLock is reentrant for read-read and write-write, acquiring a read lock while holding a write lock is allowed for this implementation, but it is fragile and indicates a design inconsistency: isOpen() should be a private helper that assumes the lock is already held, or the lock should be explicitly downgraded. More critically, if this were changed to a different lock implementation later, it would deadlock. The surrounding code already holds lock.writeLock(), so isOpen()'s internal read lock acquisition is redundant and risky.

Suggested fix:

Suggested change
throw new IllegalArgumentException("Video file is not readable: " + file);
if (currentPath != null && grabber != null && started) {
close();
}

}
if (file.toString().trim().isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
Comment on lines +76 to +84

@devin-ai-integration devin-ai-integration Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Empty-path validation is ordered after filesystem checks, causing wrong error message

The empty-path check at line 82 is placed after Files.exists(file) / Files.isReadable(file) checks at lines 76-81. On Linux/Unix, Path.of("") resolves to the current working directory, so Files.exists(Path.of("")) returns true. Line 76 correctly guards against this with !file.toString().trim().isEmpty(), but line 79 has no such guard and calls Files.exists(file) unconditionally. If the current directory exists but is not readable, line 79 throws "Video file is not readable" instead of the correct "Video file path cannot be empty" from line 82, producing a misleading error message.

Suggested change
if (!file.toString().trim().isEmpty() && !Files.exists(file)) {
throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (Files.exists(file) && !Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);
}
if (file.toString().trim().isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
if (file.toString().trim().isEmpty()) {
throw new IllegalArgumentException("Video file path cannot be empty");
}
if (!Files.exists(file)) {
throw new IllegalArgumentException("Video file does not exist: " + file);
}
if (!Files.isReadable(file)) {
throw new IllegalArgumentException("Video file is not readable: " + file);
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +76 to +84

@devin-ai-integration devin-ai-integration Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Using trim() to detect empty paths may reject valid whitespace-named files

The validation uses file.toString().trim().isEmpty() to check for empty paths. On Unix/Linux, a file can legitimately be named with whitespace characters (e.g., Path.of(" ")). Using trim() means such paths would be rejected as "empty" even though they refer to a real (if unusual) filename. Using file.toString().isEmpty() would be more precise, though whitespace-only filenames are extremely rare in practice.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


if (isOpen()) {
close();
}
Comment on lines 86 to 88

@devin-ai-integration devin-ai-integration Bot May 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: ReentrantReadWriteLock reentrancy is safe here

The open() method holds the write lock and calls isOpen() (line 86) which acquires the read lock, and close() (line 87) which acquires the write lock. This initially looks like a potential deadlock, but Java's ReentrantReadWriteLock explicitly supports reentrancy: a thread holding the write lock can re-acquire either the write lock or the read lock. So this is safe.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Expand Down
Loading