Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the roh-viz helper script to stream bcftools roh output via zcat instead of zless, aiming to remove the dependency on less in minimal/container environments.
Changes:
- Replace
zlesspipe withzcatwhen reading the RoH input file. - Update the corresponding close error message (though it currently references the wrong option key).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| print STDERR "Parsing $$opts{outdir}/roh.txt\n"; | ||
| open(my $in,"zless $$opts{roh_file} |") or error("zless $$opts{roh_file}: $!"); | ||
| open(my $in,"zcat $$opts{roh_file} |") or error("zcat $$opts{roh_file}: $!"); |
There was a problem hiding this comment.
Using zcat here will fail for the common case where --RoH-file is plain text (the help example uses roh.txt, not a .gz). zless can view uncompressed files, but zcat typically errors with “not in gzip format” and produces no parsed data. Consider opening the file directly when it’s not gzipped (e.g., based on extension or magic bytes), and only using a decompressor for .gz inputs (or use IO::Uncompress::Gunzip for gzip with a plain-file fallback).
| } | ||
| } | ||
| close($in) or error("close failed: zless $$opts{in_file}"); | ||
| close($in) or error("close failed: zcat $$opts{in_file}"); |
There was a problem hiding this comment.
The close error message references $$opts{in_file}, but this function opens $$opts{roh_file} and there is no in_file option in this script. This will produce a misleading/empty error message if the pipe fails; update it to refer to the RoH input and ideally include the underlying failure info (e.g., $! and/or $?).
| close($in) or error("close failed: zcat $$opts{in_file}"); | |
| close($in) or error("close failed: zcat $$opts{roh_file} (status=$?, error=$!)"); |
|
|
||
| print STDERR "Parsing $$opts{outdir}/roh.txt\n"; | ||
| open(my $in,"zless $$opts{roh_file} |") or error("zless $$opts{roh_file}: $!"); | ||
| open(my $in,"zcat $$opts{roh_file} |") or error("zcat $$opts{roh_file}: $!"); |
There was a problem hiding this comment.
Using one-arg open with a piped string command (open(my $in, "zcat $$opts{roh_file} |")) invokes a shell and lets it interpret $$opts{roh_file}, enabling OS command injection if that value is attacker-controlled. Here roh_file comes directly from CLI arguments, so a crafted filename containing shell metacharacters could execute arbitrary commands with this script’s privileges. Replace this with a shell-free invocation (e.g., multi-arg open or a decompression module that takes the filename as a separate argument) and/or strictly validate the allowed pathname characters.
| open(my $in,"zcat $$opts{roh_file} |") or error("zcat $$opts{roh_file}: $!"); | |
| open(my $in, '-|', 'zcat', $$opts{roh_file}) or error("zcat $$opts{roh_file}: $!"); |
Hi.
Reading the code, I think you don't need the "less" capability in
roh-viz. I think the script just needs a plain-text stream of a potentially-compressed input file.zcatdoes just that.It makes a difference in some containers where
lessisn't necessarily installed.Best regards,
Matthieu