fix(VM_Manager): fix gpf issues with snapshots and php 8.4#2537
fix(VM_Manager): fix gpf issues with snapshots and php 8.4#2537limetech merged 1 commit intounraid:masterfrom
Conversation
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
WalkthroughThe changes modify snapshot handling in the libvirt helpers by introducing a fallback XML retrieval mechanism that attempts filesystem access before invoking virsh commands, and replacing certain direct library calls with shell command execution for snapshot operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php (1)
1889-1919:⚠️ Potential issue | 🟠 MajorSanitize snapshot path + validate XML before parsing.
$vm/$nameare interpolated into a filesystem path without validation, enabling path traversal; additionally, any non-empty but invalid XML will makesimplexml_load_stringfail and later array indexing will raise warnings and corrupt snapshot DB entries.🛡️ Proposed hardening (path validation + XML sanity)
- $snapshot_xml = `@file_get_contents`("/etc/libvirt/qemu/snapshot/{$vm}/{$name}.xml"); + // Restrict path segments to safe snapshot identifiers + if (!preg_match('/^[A-Za-z0-9._-]+$/', $vm) || !preg_match('/^[A-Za-z0-9._-]+$/', $name)) { + $snapshot_xml = ''; + } else { + $snapshot_xml = `@file_get_contents`("/etc/libvirt/qemu/snapshot/{$vm}/{$name}.xml"); + } if (empty($snapshot_xml)) { $snapshot_xml = trim(shell_exec("virsh snapshot-dumpxml ".escapeshellarg($vm)." ".escapeshellarg($name)." 2>/dev/null")); } - $snapshot_res = !empty($snapshot_xml); + // Validate XML before use + if (!empty($snapshot_xml)) { + libxml_use_internal_errors(true); + $snapshot_obj = simplexml_load_string($snapshot_xml); + if ($snapshot_obj === false) { + $snapshot_xml = ''; + } + libxml_clear_errors(); + } + $snapshot_res = !empty($snapshot_xml);
🤖 Fix all issues with AI agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php`:
- Around line 2140-2142: The snapshot-delete exec call ignores the return status
and can leave stale metadata; inside the if ($ret != "noxml") block where
exec("virsh snapshot-delete ...", $snapDelOut, $snapDelRtn) is called, check
$snapDelRtn for non-zero, log the error including $snapDelRtn and
implode("\n",$snapDelOut) for details, and fail the operation (return false or
throw) or retry/cleanup as appropriate so you do not proceed as if metadata was
removed when it wasn't; ensure any subsequent code that removes or updates
metadata only runs when $snapDelRtn === 0.
| if ($ret != "noxml") { | ||
| exec("virsh snapshot-delete ".escapeshellarg($vm)." ".escapeshellarg($name)." --metadata 2>&1", $snapDelOut, $snapDelRtn); | ||
| } |
There was a problem hiding this comment.
Handle snapshot-delete failures.
exec() return status is ignored; if virsh snapshot-delete --metadata fails, you silently keep stale metadata.
✅ Suggested error handling
if ($ret != "noxml") {
- exec("virsh snapshot-delete ".escapeshellarg($vm)." ".escapeshellarg($name)." --metadata 2>&1", $snapDelOut, $snapDelRtn);
+ exec("virsh snapshot-delete ".escapeshellarg($vm)." ".escapeshellarg($name)." --metadata 2>&1", $snapDelOut, $snapDelRtn);
+ if ($snapDelRtn !== 0) {
+ if ($logging) qemu_log($vm, "snapshot-delete failed: ".implode("\n", $snapDelOut));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($ret != "noxml") { | |
| exec("virsh snapshot-delete ".escapeshellarg($vm)." ".escapeshellarg($name)." --metadata 2>&1", $snapDelOut, $snapDelRtn); | |
| } | |
| if ($ret != "noxml") { | |
| exec("virsh snapshot-delete ".escapeshellarg($vm)." ".escapeshellarg($name)." --metadata 2>&1", $snapDelOut, $snapDelRtn); | |
| if ($snapDelRtn !== 0) { | |
| if ($logging) qemu_log($vm, "snapshot-delete failed: ".implode("\n", $snapDelOut)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@emhttp/plugins/dynamix.vm.manager/include/libvirt_helpers.php` around lines
2140 - 2142, The snapshot-delete exec call ignores the return status and can
leave stale metadata; inside the if ($ret != "noxml") block where exec("virsh
snapshot-delete ...", $snapDelOut, $snapDelRtn) is called, check $snapDelRtn for
non-zero, log the error including $snapDelRtn and implode("\n",$snapDelOut) for
details, and fail the operation (return false or throw) or retry/cleanup as
appropriate so you do not proceed as if metadata was removed when it wasn't;
ensure any subsequent code that removes or updates metadata only runs when
$snapDelRtn === 0.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.