update generate video for proper escaping - #4975
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Event::GenerateVideo() (ZoneMinder web UI backend) to improve shell escaping around the ffmpeg invocation so that ffmpeg’s output can be redirected into ffmpeg.log, addressing Issue #4970 where the UI references a log file that isn’t created.
Changes:
- Switches to argument-level escaping (
escapeshellarg) for ffmpeg input/output paths and filter arguments. - Stops wrapping the entire command with
escapeshellcmd, allowing shell redirection (> ... 2>&1) to function. - Expands the error log message to include the captured
exec()output and return code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot recommended some fixes and it feels like all the ffmpeg errors are gone. However, I still get an internal server error (500 from the server) but I get no errors in the system logs. So there must be something else and I am not sure where to go from here. |
|
In system logs or in apache log? Typically with 500, there would be something in /var/log/apache2/error.log (or site-specific log). Maybe check journalctl -f ? Changes look good to me. |
| $command = ZM_PATH_FFMPEG | ||
| ." -y -r $frame_rate " | ||
| .ZM_FFMPEG_INPUT_OPTIONS | ||
| .' -i ' . $event_path.'/'.( $this->DefaultVideo() ? $this->DefaultVideo() : '%0'.ZM_EVENT_IMAGE_DIGITS .'d-capture.jpg' ) | ||
| ." -i ".escapeshellarg($event_path.'/'.( $this->DefaultVideo() ? $this->DefaultVideo() : '%0'.ZM_EVENT_IMAGE_DIGITS .'d-capture.jpg' )) |
| .implode(' ', array_map(function($t){ return ' -vf '.escapeshellarg($t); }, array_filter(explode(',', $transforms), 'strlen'))) | ||
| ." -s $video_size " | ||
|
|
||
| .ZM_FFMPEG_OUTPUT_OPTIONS | ||
| ." '$event_path/$video_file' > $event_path/ffmpeg.log 2>&1" | ||
| ." ".escapeshellarg($event_path.'/'.$video_file)." > ".escapeshellarg($event_path.'/ffmpeg.log')." 2>&1" |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
I checked system logs inside ZM. This is what I see from apache logs which seems like a PHP error:
This will be for a separate another PR. Edit: I tried adding a |
|
Checking if anything is preventing this from being merged? |
|
There are a bunch of AI reviews... which may or may not be valid, but must be defended. I've been meaning to have another AI do the defending.. but dayjob and life are getting in the way. |
|
The majority of the AI fixes is how I was able to get further but stuck at #4970 (comment) |
Issue #4970
This just fixes the log file creation. It doesn't resolve the issue.
I do know PHP, but it has been a while and because of the complexity of the command I asked AI to assist with the changes.
Feel free to review and recommend any updates.