Skip to content

Automate nbdkit data integrity test for NBD request size constraints and NFS truncation risk #6843

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

Closed
ganeshhubale wants to merge 2 commits into from
Closed

Automate nbdkit data integrity test for NBD request size constraints and NFS truncation risk #6843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0bef1
Automate nbdkit data integrity test for NBD request size constraints …
ganeshhubale Mar 25, 2026
7cd3eb9
Merge branch 'master' into auto-data-exposure-nfs
ganeshhubale Mar 30, 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
30 changes: 19 additions & 11 deletions v2v/tests/cfg/nbdkit/nbdkit.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,19 +148,27 @@
external_image_url = EXTERNAL_IMAGE_FAKE_URL
- nbdcopy:
only source_none..dest_none
version_required = "[nbdkit-server-1.42.2-4,);[libnbd-1.22.2-2,)"
disk_path = DEFAULT_TEST_IMAGE
checkpoint = 'check_blkhash_option'
variants:
- blkhash:
variants:
- positive_test:
test_state= 'positive'
hash_option= 'md5/512k'
- negative_test:
test_state= 'negative'
hash_option= 'sha1/100k'
expected_warn_msg= 'nbdcopy: --blkhash is not a power of two: 100k'
version_required = "[nbdkit-server-1.42.2-4,);[libnbd-1.22.2-2,)"
disk_path = DEFAULT_TEST_IMAGE
checkpoint = 'check_blkhash_option'
variants:
- positive_test:
test_state= 'positive'
hash_option= 'md5/512k'
- negative_test:
test_state= 'negative'
hash_option= 'sha1/100k'
expected_warn_msg= 'nbdcopy: --blkhash is not a power of two: 100k'
- data_integrity:
checkpoint = 'check_data_integrity'
version_required = "[nbdkit-1.46,)"
variants:
- positive_test:
test_state= 'positive'
- negative_test:
test_state= 'negative'
Comment thread
ganeshhubale marked this conversation as resolved.
- blocksize:
only source_none..dest_none
version_required = "[nbdkit-1.38.0-1,)"
Expand Down
64 changes: 64 additions & 0 deletions v2v/tests/src/nbdkit/nbdkit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,68 @@ def test_nbdkit_instance_name():
# Force kill if it's being stubborn
p.kill()

def test_nbd_data_integrity():
from subprocess import Popen, PIPE, STDOUT
import time
# --- Configuration ---
SOCKET = "/tmp/test.sock"
IMG_PATH = os.path.join(data_dir.get_tmp_dir(), "test.img")
IMAGE_SIZE = "1G"

LOG.info(f"Prepare {IMAGE_SIZE} image file")
cmd = f"truncate -s {IMAGE_SIZE} {IMG_PATH}"
process.run(cmd, shell=True, ignore_status=True)
if os.path.exists(SOCKET):
os.remove(SOCKET)

LOG.info("Starting nbdkit server in background")
nbd_log_file = os.path.join(data_dir.get_tmp_dir(), "nbd_log_file.log")
# Start nbdkit with debug flags enabled
with open(nbd_log_file, "w") as nbd_log_file:
server_cmd = ["nbdkit", "-f", "-v", "-D", "file.zero=1", "-U", SOCKET, "file", IMG_PATH]
nbd_server = Popen(server_cmd, stdout=nbd_log_file, stderr=nbd_log_file)
# Wait for the socket file to appear
time.sleep(1)
Comment thread
ganeshhubale marked this conversation as resolved.
Comment on lines +816 to +833
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use a per-run UNIX socket and wait for it explicitly.

The fixed /tmp/test.sock can clash with another avocado job, and the unconditional os.remove(SOCKET) can unlink a socket that belongs to a different still-running test. On top of that, sleep(1) makes startup flaky on slower hosts. Please allocate a short unique socket path under /tmp and poll for socket creation or early nbdkit exit before running nbdcopy.

🧰 Tools
🪛 Ruff (0.15.6)

[error] 816-816: Probable insecure usage of temporary file or directory: "/tmp/test.sock"

(S108)

[error] 822-822: Function call with shell=True parameter identified, security issue

(S604)

[error] 831-831: subprocess call: check for execution of untrusted input

(S603)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 816 - 833, Replace the fixed
SOCKET and blind sleep with a per-run unique UNIX socket and an explicit wait
loop: create SOCKET using a unique name (e.g. include pid/uuid) instead of
"/tmp/test.sock", avoid unconditional os.remove(SOCKET), start nbdkit via Popen
(the existing nbd_server) and then poll in a short loop that (a) checks for
SOCKET existence, (b) checks nbd_server.poll() for early exit and logs/raises if
it exited, and (c) times out after a short period failing the test; update the
startup block that defines SOCKET, starts nbd_server, and the time.sleep(1) to
this poll-and-check logic so callers of nbdkit (e.g. nbdcopy) only proceed when
the socket is owned by this run and nbdkit is alive.


test_state = params_get(params, "test_state")
try:
if test_state == "positive":
LOG.info("Testing nbdcopy with 32M")
cmd = f"nbdcopy --request-size=32M /dev/zero nbd+unix:///?socket={SOCKET}"
result = process.run(cmd, shell=True, ignore_status=True)
exp_warn = "No space left on device"
if exp_warn not in result.stderr_text:
test.fail(f"Expected warning {exp_warn}, got {result.stderr_text}")

LOG.info("Verification (Compare with /dev/zero)")
cmd = f"nbdcopy nbd+unix:///?socket={SOCKET} - | cmp -n {IMAGE_SIZE} - /dev/zero"
result = process.run(cmd, shell=True, ignore_status=True)

if result.exit_status != 0:
test.fail(f"Check Failed: Mismatch found! {result.stderr_text}")

LOG.info("Verification (MD5 Hash Check)")
# Pipe the output directly into a hash to save memory
hash_cmd = f"nbdcopy nbd+unix:///?socket={SOCKET} - | head -c {IMAGE_SIZE} | md5sum"
result = process.run(hash_cmd, shell=True, ignore_status=True)
actual_md5 = result.stdout_text
result = process.run("head -c 1G /dev/zero | md5sum", shell=True, ignore_status=True)
expected_md5 = result.stdout_text
if actual_md5 != expected_md5:
test.fail("[FAILURE] Data corruption detected.")
elif test_state == "negative":
LOG.info("Testing nbdcopy with 128M (Expected Failure)")
cmd = f"nbdcopy --request-size=128M /dev/zero nbd+unix:///?socket={SOCKET}"
result = process.run(cmd, shell=True, ignore_status=True)
err_msg = "must be a power of 2 within 4096-33554432: 128M"
if err_msg not in result.stderr_text:
test.fail(f"Expected warning {err_msg}, got {result.stderr_text}")
finally:
LOG.info("Cleaning up")
nbd_server.terminate()
nbd_server.wait()
if os.path.exists(SOCKET):
os.remove(SOCKET)
Comment on lines +868 to +873
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 25, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "nbdkit.py" | head -5

Repository: autotest/tp-libvirt

Length of output: 95

🏁 Script executed:

cd v2v/tests/src/nbdkit && wc -l nbdkit.py

Repository: autotest/tp-libvirt

Length of output: 76

🏁 Script executed:

cd v2v/tests/src/nbdkit && sed -n '860,880p' nbdkit.py

Repository: autotest/tp-libvirt

Length of output: 1065

🏁 Script executed:

cd v2v/tests/src/nbdkit && grep -n "test_nbdkit_instance_name" nbdkit.py

Repository: autotest/tp-libvirt

Length of output: 199

🏁 Script executed:

cd v2v/tests/src/nbdkit && sed -n '742,810p' nbdkit.py

Repository: autotest/tp-libvirt

Length of output: 3108

🏁 Script executed:

cd v2v/tests/src/nbdkit && sed -n '800,820p' nbdkit.py

Repository: autotest/tp-libvirt

Length of output: 846

Bound subprocess shutdown with timeout.

nbd_server.wait() can hang the whole job if nbdkit stops responding. This should use the same terminate/wait-with-timeout/kill fallback pattern already implemented in test_nbdkit_instance_name().

🛠️ Proposed fix 
-        from subprocess import Popen, PIPE, STDOUT
+        from subprocess import Popen, TimeoutExpired
...
         finally:
             LOG.info("Cleaning up")
-            nbd_server.terminate()
-            nbd_server.wait()
+            if nbd_server.poll() is None:
+                nbd_server.terminate()
+                try:
+                    nbd_server.wait(timeout=5)
+                except TimeoutExpired:
+                    nbd_server.kill()
+                    nbd_server.wait(timeout=5)
             if os.path.exists(SOCKET):
                 os.remove(SOCKET)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@v2v/tests/src/nbdkit/nbdkit.py` around lines 868 - 873, The finally block
currently calls nbd_server.terminate() followed by a blocking nbd_server.wait(),
which can hang; update the cleanup to mirror the
terminate/wait-with-timeout/kill fallback used in test_nbdkit_instance_name():
after calling nbd_server.terminate(), call nbd_server.wait(timeout=some_seconds)
and if it times out or the process is still alive call nbd_server.kill() and
wait again (with a short timeout), catching and logging exceptions via LOG;
still remove SOCKET if present. Reference nbd_server.terminate(),
nbd_server.wait(), nbd_server.kill(), LOG, SOCKET and reuse the same timeout
semantics as test_nbdkit_instance_name().

def test_count_filter():
# p is our SubProcess object
p = None
Expand Down Expand Up @@ -969,6 +1031,8 @@ def test_data_plugin_supports_base64_in_format_string():
check_blocksize_constraints()
elif checkpoint == 'test_nbdkit_instance_name':
test_nbdkit_instance_name()
elif checkpoint == 'check_data_integrity':
test_nbd_data_integrity()
elif checkpoint == 'test_count_filter':
test_count_filter()
elif checkpoint == 'test_data_plugin_supports_base64_in_format_string':
Expand Down
Loading