Massive fix - #6915
Conversation
Libvirt now rejects VLAN tags on direct/bridge at define time (virDomainNetDefValidate), so the negative test must use define_error instead of start_error.
aarch64 EFI guests often already use per-device disk boot and lack os/boot, so attach failed with a duplicate boot-order error instead of the expected os/boot conflict. Prepare the domain like the newer attach_iface_with_boot_order test.
CI guests are created with type=bridge (virbr0). Avocado drops an iface_type cfg key, so set type=network in code when direct_net is used. Otherwise update-device tries to change the interface type and fails with "unable to change config on 'network' network type".
aarch64 EFI guests already have per-device disk boot, so update-device with iface boot=1 failed with a duplicate boot-order error instead of the expected os/boot conflict. Detect via expect_err_msg (avocado may drop unknown cfg keys) and set os/boot like iface_attach_detach.
RHEL 10 no longer ships dhcp-client; install dhcpcd first and fall back to dhcp-client for older guests. Committer: Bolatbek Issakh <bissakh@redhat.com>
WalkthroughVirtual network tests now configure boot-order preconditions before interface modification and expect the macvtap VLAN failure during definition. Interface updates preserve type information and prepare boot and direct-network attributes before applying changes. Guest network setup in migration, multivm, and SR-IOV tests now accepts either Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/migration/migrate_network.py`:
- Around line 320-324: Update the post-migration DHCP handling around the
package_install fallback to retain which client was successfully installed, then
ensure the restart paths that currently invoke restart_dhclient use matching
dhcpcd logic or the client-agnostic utils_net.restart_guest_network helper.
Preserve the existing dhcp-client behavior while making dhcpcd-only guests
recover successfully.
In `@libvirt/tests/src/virtual_network/iface_update.py`:
- Around line 250-252: Replace the unsafe eval() call in the
update_driver_iommu_ast case with ast.literal_eval() when parsing
params.get('iommu_attrs', '{}'), and ensure the ast module is imported. Preserve
the resulting iommu_attrs value and the existing
libvirt_virtio.add_iommu_dev(vm, iommu_attrs) call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80c08af1-3791-40e2-8d8e-625b06991c84
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/iface_network.cfglibvirt/tests/src/migration/migrate_network.pylibvirt/tests/src/virtual_network/iface_attach_detach.pylibvirt/tests/src/virtual_network/iface_update.pylibvirt/tests/src/virtual_network/virtual_network_multivms.pyprovider/sriov/sriov_base.py
| # RHEL <= 9: dhcp-client; RHEL 10+: dhcpcd | ||
| if not (utils_package.package_install('dhcpcd', session=vm_session) or | ||
| utils_package.package_install('dhcp-client', session=vm_session)): | ||
| test.error("Failed to install dhcp client (dhcpcd/dhcp-client) on guest.") | ||
| utils_net.restart_guest_network(vm_session) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the DHCP client that was actually installed after migration.
This fallback can select dhcpcd, but the later post-migration paths still execute restart_dhclient (defaulting to dhclient -r; dhclient at Lines 352 and 402). On a dhcpcd-only guest, network recovery can therefore fail even though installation succeeded. Retain the selected client and use matching restart logic, or call the client-agnostic network restart helper in those paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/migration/migrate_network.py` around lines 320 - 324,
Update the post-migration DHCP handling around the package_install fallback to
retain which client was successfully installed, then ensure the restart paths
that currently invoke restart_dhclient use matching dhcpcd logic or the
client-agnostic utils_net.restart_guest_network helper. Preserve the existing
dhcp-client behavior while making dhcpcd-only guests recover successfully.
| if case == 'update_driver_iommu_ast': | ||
| iommu_attrs = eval(params.get('iommu_attrs', '{}')) | ||
| libvirt_virtio.add_iommu_dev(vm, iommu_attrs) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 "iommu_attrs\s*=" libvirt/tests/cfg libvirt/tests/srcRepository: autotest/tp-libvirt
Length of output: 1682
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline libvirt/tests/src/virtual_network/iface_update.py --view expanded | sed -n '1,220p' || true
echo "== top of iface_update.py =="
sed -n '1,40p' libvirt\Tests/src/virtual_network/iface_update.py 2>/dev/null || sed -n '1,40p' libvirt/tests/src/virtual_network/iface_update.py
echo "== cfg section around driver ast =="
sed -n '170,195p' libvirt/tests/cfg/virtual_network/iface_update.cfg
echo "== all eval()/literals parser related in target files =="
rg -n "\b(eval|literal_eval|params\.get|--cfg)\b" libvirt/tests/cfg/virtual_network/iface_update.cfg libvirt/tests/src/virtual_network/iface_update.pyRepository: autotest/tp-libvirt
Length of output: 8644
🌐 Web query:
Python ast.literal_eval security safe parsing literals eval arbitrary code execution documentation
💡 Result:
The function ast.literal_eval is designed to parse and evaluate strings containing only basic Python literal structures (e.g., strings, numbers, tuples, lists, dicts, sets, booleans, None, and Ellipsis) [1][2][3]. While it is fundamentally different from eval because it does not execute arbitrary Python code, it is not considered inherently "safe" for processing untrusted input [1][4]. Key security considerations include: 1. No Arbitrary Code Execution: Unlike eval, ast.literal_eval does not maintain a namespace, perform name lookups, or allow the invocation of functions or methods, which prevents direct arbitrary code execution [1][5]. 2. Denial of Service (DoS) Risks: The function is vulnerable to denial-of-service attacks when processing untrusted input [1]. A relatively small, specially crafted string can trigger excessive memory consumption, excessive CPU usage, or C stack exhaustion, leading to a crash of the Python interpreter [1][4][5]. 3. Documentation Updates: Historically, the Python documentation described ast.literal_eval as "safe," but this was determined to be misleading [1][6][3]. Official documentation has been updated to remove this guarantee and explicitly warns against using the function with untrusted sources [1][4]. For scenarios requiring the parsing of untrusted data, using a safer, non-executable format like JSON (via the json module) is generally recommended [7]. If ast.literal_eval must be used, input should be validated or restricted (e.g., by limiting the input length) to mitigate potential DoS risks [6].
Citations:
- 1: https://docs.python.org/3.10/library/ast.html
- 2: https://docs.python.org/3/library/ast.html
- 3: https://docs.python.org/3.9/library/ast.html
- 4: gh-95588: Drop the safety claim from
ast.literal_evaldocs. python/cpython#95919 - 5: https://stackoverflow.com/questions/4710247/python-3-are-there-any-known-security-holes-in-ast-literal-evalnode-or-string
- 6: Rephrase ast.literal_eval() to remove any security warranty python/cpython#95588
- 7: https://stackoverflow.com/questions/15197673/using-pythons-eval-vs-ast-literal-eval
Do not execute iommu_attrs parameter text.
params is populated from test configuration, so eval() can execute arbitrary Python if the parameter is malformed or untrusted. Use ast.literal_eval() so only Python literals are accepted.
Proposed fix
+import ast
+
- iommu_attrs = eval(params.get('iommu_attrs', '{}'))
+ iommu_attrs = ast.literal_eval(params.get('iommu_attrs', '{}'))📝 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 case == 'update_driver_iommu_ast': | |
| iommu_attrs = eval(params.get('iommu_attrs', '{}')) | |
| libvirt_virtio.add_iommu_dev(vm, iommu_attrs) | |
| import ast | |
| if case == 'update_driver_iommu_ast': | |
| iommu_attrs = ast.literal_eval(params.get('iommu_attrs', '{}')) | |
| libvirt_virtio.add_iommu_dev(vm, iommu_attrs) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 250-250: use of eval can be insecure
Context: eval(params.get('iommu_attrs', '{}'))
Note: [CWE-94] Improper Control of Generation of Code ('Code Injection').
(no-eval-python)
🪛 Ruff (0.15.21)
[error] 251-251: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libvirt/tests/src/virtual_network/iface_update.py` around lines 250 - 252,
Replace the unsafe eval() call in the update_driver_iommu_ast case with
ast.literal_eval() when parsing params.get('iommu_attrs', '{}'), and ensure the
ast module is imported. Preserve the resulting iommu_attrs value and the
existing libvirt_virtio.add_iommu_dev(vm, iommu_attrs) call.
Sources: Learnings, Linters/SAST tools
1 participant
Summary by CodeRabbit
Bug Fixes
dhcpcdordhcp-client.Tests