fix(bam): refuse whitespace in read names and framing bytes in tag values (#415)

CHANGELOG.md

@@ -5,6 +5,11 @@
 
 #### Changes
 
+- **A BAM read name containing whitespace, or whitespace other than a space inside a preserved
+  aux-tag value, is now refused on every output format**
+  ([#415](https://github.com/FelixKrueger/TrimGalore/issues/415)) — see the uBAM section of the
+  docs for the samtools recipe that rewrites space-bearing names.
+
 - **A paired run now writes every output to one directory**
   ([#398](https://github.com/FelixKrueger/TrimGalore/issues/398)). Without
   `--output_dir`, the validated FASTQs already went to Read 1's directory, but each

docs/src/content/docs/quickstart.md

@@ -67,6 +67,20 @@ trim_galore --output-format ubam --preserve-tags CB,UB sample.bam
 
 Paired uBAM output is a single interleaved BAM (`<stem>_val.bam`), matching samtools/Picard/fgbio convention. See [Output files](/guide/outputs/#ubam-output---output-format-ubam) for the full contract.
 
+### Read names containing whitespace
+
+A read name containing whitespace is refused, **on every output format** — including FASTQ output, where a space or tab loses nothing today but a newline silently desynchronises the whole file. Trim Galore reads a FASTQ header as ending at the first whitespace, so an aux-tag tail after it cannot be carried, and a newline splits one record across five lines. Whitespace other than a space inside a preserved aux-tag value is refused for the same reason; a space in a tag value is legal and unaffected.
+
+The SAM specification forbids whitespace in QNAME, but samtools will build such a file. For names containing spaces only, samtools can rewrite them:
+
+```bash
+samtools view -h in.bam \
+  | awk 'BEGIN{FS=OFS="\t"} /^@/{print; next} {gsub(/ /,"_",$1); print}' \
+  | samtools view -b -o fixed.bam -
+```
+
+`FS="\t"` is essential. Without it `$1` stops at the first space, the substitution matches nothing, and the file comes out unchanged — `samtools view -b` accepts it and Trim Galore still refuses it. A name containing a tab or newline cannot be represented in SAM text at all, so it has to be corrected at source.
+
 ## Common combinations
 
 ```bash

examples/mk_bam_fixture.rs

@@ -0,0 +1,131 @@
+//! Emit the uBAM fixtures that SAM text cannot express (#415).
+//!
+//! BAM stores QNAME and a `Z` tag value as NUL-terminated byte fields, so either can hold a
+//! newline; SAM text is line-delimited and cannot, so samtools refuses to build these. noodles'
+//! record encoder rejects any read name outside `[!-?A-~]{1,254}`, so the records are packed
+//! here by hand and only the BGZF framing comes from `bgzf::Writer`.
+//!
+//! ```sh
+//! cargo run --quiet --example mk_bam_fixture -- lf_qname > test_files/ubam_lf_qname.bam
+//! ```
+
+use std::io::Write;
+
+use noodles::bgzf;
+
+const SEQ: &str = "ACGTACGTACGTACGTACGTACGTACGTACGTACGTACGT";
+const QUAL_PHRED: u8 = 40;
+const FLAG_UNMAPPED: u16 = 4;
+
+/// 4-bit sequence alphabet, indexed by the packed nibble value.
+const SEQ_CODES: &[u8] = b"=ACMGRSVTWYHKDBN";
+
+const CASES: &[&str] = &["lf_qname", "lf_tagvalue", "lf_atag", "atag_ok"];
+
+/// Two bases per byte, high nibble first.
+fn pack_seq(seq: &str) -> Vec<u8> {
+    let code = |b: u8| {
+        SEQ_CODES
+            .iter()
+            .position(|&c| c == b)
+            .unwrap_or_else(|| panic!("{} is not a IUPAC base", b as char)) as u8
+    };
+    seq.as_bytes()
+        .chunks(2)
+        .map(|pair| (code(pair[0]) << 4) | pair.get(1).map_or(0, |&b| code(b)))
+        .collect()
+}
+
+/// One aux field: two-char tag, `Z`, NUL-terminated value.
+fn z_tag(tag: &str, value: &[u8]) -> Vec<u8> {
+    let mut out = tag.as_bytes().to_vec();
+    out.push(b'Z');
+    out.extend_from_slice(value);
+    out.push(0);
+    out
+}
+
+/// One aux field: two-char tag, `A`, a single unterminated byte.
+fn a_tag(tag: &str, byte: u8) -> Vec<u8> {
+    let mut out = tag.as_bytes().to_vec();
+    out.push(b'A');
+    out.push(byte);
+    out
+}
+
+/// One unmapped BAM record: `block_size` then the 32-byte fixed block, name, seq, qual, aux.
+fn record(name: &[u8], aux: &[u8]) -> Vec<u8> {
+    let mut name_z = name.to_vec();
+    name_z.push(0);
+
+    let mut body = Vec::new();
+    body.extend_from_slice(&(-1i32).to_le_bytes()); // ref_id
+    body.extend_from_slice(&(-1i32).to_le_bytes()); // pos
+    body.push(u8::try_from(name_z.len()).expect("read name fits in l_read_name"));
+    body.push(0); // mapq
+    body.extend_from_slice(&4680u16.to_le_bytes()); // bin
+    body.extend_from_slice(&0u16.to_le_bytes()); // n_cigar_op
+    body.extend_from_slice(&FLAG_UNMAPPED.to_le_bytes());
+    body.extend_from_slice(&(SEQ.len() as i32).to_le_bytes()); // l_seq
+    body.extend_from_slice(&(-1i32).to_le_bytes()); // next_ref_id
+    body.extend_from_slice(&(-1i32).to_le_bytes()); // next_pos
+    body.extend_from_slice(&0i32.to_le_bytes()); // tlen
+    body.extend_from_slice(&name_z);
+    body.extend_from_slice(&pack_seq(SEQ));
+    body.extend_from_slice(&vec![QUAL_PHRED; SEQ.len()]);
+    body.extend_from_slice(aux);
+
+    let mut out = (body.len() as i32).to_le_bytes().to_vec();
+    out.extend_from_slice(&body);
+    out
+}
+
+/// The offending record sits second, so record 1 clears the sanity-check peek and the
+/// failure lands in the trimming loop.
+fn case_records(case: &str) -> Option<Vec<(Vec<u8>, Vec<u8>)>> {
+    let clean_cb = || z_tag("CB", b"AAACCC");
+    Some(match case {
+        "lf_qname" => vec![
+            (b"readA".to_vec(), clean_cb()),
+            (b"readB\nEVIL".to_vec(), clean_cb()),
+            (b"readC".to_vec(), clean_cb()),
+        ],
+        "lf_tagvalue" => vec![
+            (b"readA".to_vec(), clean_cb()),
+            (b"readB".to_vec(), z_tag("CB", b"AAA\nCCC")),
+            (b"readC".to_vec(), clean_cb()),
+        ],
+        "lf_atag" => vec![
+            (b"readA".to_vec(), a_tag("XA", b'+')),
+            (b"readB".to_vec(), a_tag("XA", b'\n')),
+            (b"readC".to_vec(), a_tag("XA", b'+')),
+        ],
+        // The acceptance twin for the `A` arm: nothing here may be refused.
+        "atag_ok" => vec![
+            (b"readA".to_vec(), a_tag("XA", b'+')),
+            (b"readB".to_vec(), a_tag("XA", b'-')),
+            (b"readC".to_vec(), a_tag("XA", b'+')),
+        ],
+        _ => return None,
+    })
+}
+
+fn main() -> std::io::Result<()> {
+    let case = std::env::args().nth(1).unwrap_or_default();
+    let Some(records) = case_records(&case) else {
+        eprintln!("usage: mk_bam_fixture <{}>", CASES.join("|"));
+        std::process::exit(2);
+    };
+
+    let header_text = b"@HD\tVN:1.6\n";
+    let mut writer = bgzf::Writer::new(std::io::stdout().lock());
+    writer.write_all(b"BAM\x01")?;
+    writer.write_all(&(header_text.len() as i32).to_le_bytes())?;
+    writer.write_all(header_text)?;
+    writer.write_all(&0i32.to_le_bytes())?; // n_ref
+    for (name, aux) in &records {
+        writer.write_all(&record(name, aux))?;
+    }
+    let _stdout = writer.finish()?;
+    Ok(())
+}

src/bam.rs

@@ -862,6 +862,49 @@ pub fn peek_header(path: &Path) -> Result<Header> {
     Ok(header)
 }
 
+/// Reject a BAM read name holding a byte that breaks the FASTQ id it becomes.
+///
+/// The chosen set is `u8::is_ascii_whitespace` — what `parse_name_and_data`,
+/// `fastq::read_id_prefix` and the id's one-line framing split on. VT sits deliberately
+/// outside it; `append_to_id`'s `trim_end` does strip a trailing one.
+fn reject_whitespace_in_qname(name: &[u8]) -> Result<()> {
+    if name.iter().any(u8::is_ascii_whitespace) {
+        let shown = String::from_utf8_lossy(name);
+        bail!(
+            "BAM read name contains whitespace: \"{}\". Trim Galore reads a FASTQ header as \
+             ending at the first whitespace, so any --preserve-tags aux tags after it are lost \
+             on uBAM output, and a newline would break the FASTQ record structure. The SAM \
+             specification forbids whitespace in QNAME. Rename the reads before trimming — the \
+             uBAM section of the docs gives a samtools recipe for space-only names; a tab or \
+             newline in a name cannot be expressed in SAM text at all.",
+            shown.escape_debug()
+        );
+    }
+    Ok(())
+}
+
+/// Reject bytes in an appended `TYPE:VALUE` that would break the FASTQ id holding it.
+///
+/// The QNAME boundary set minus space, which `Z`'s `[ !-~]*` grammar makes legal: a tab
+/// would forge a tag field and a newline would break the single-line id.
+fn reject_framing_in_tag_value(text: &str) -> Result<()> {
+    if text.bytes().any(|b| b != b' ' && b.is_ascii_whitespace()) {
+        // `Z` values are unbounded, so borrow the 60-char cap the description notice uses.
+        let shown = if text.chars().count() > 60 {
+            format!("{}…", text.chars().take(60).collect::<String>())
+        } else {
+            text.to_string()
+        };
+        bail!(
+            "aux tag value cannot pass through the FASTQ header: \"{}\". A tab would forge a \
+             tag field and a newline would break the single-line read ID; spaces are legal and \
+             unaffected. Fix the tag at source.",
+            shown.escape_debug()
+        );
+    }
+    Ok(())
+}
+
 /// Convert one BAM record to a `FastqRecord`. Per-record validation:
 /// - Must be unmapped (`is_unmapped()` true; per-record check resolves
 ///   PLAN-REVIEW B-Crit-4 first-record-only contradiction).
@@ -894,6 +937,7 @@ fn bam_record_to_fastq(rec: &bam::Record, tags: &[String]) -> Result<FastqRecord
     if name.is_empty() {
         bail!("BAM record has empty read name");
     }
+    reject_whitespace_in_qname(name)?;
     let id_body = std::str::from_utf8(name).context("BAM record name is not valid UTF-8")?;
     let mut id = format!("@{}", id_body);
 
@@ -916,7 +960,8 @@ fn bam_record_to_fastq(rec: &bam::Record, tags: &[String]) -> Result<FastqRecord
                 id.push('\t');
                 id.push_str(tag_name);
                 id.push(':');
-                append_tag_type_and_value(&mut id, &value)?;
+                append_tag_type_and_value(&mut id, &value)
+                    .with_context(|| format!("aux tag '{}'", tag_name))?;
             }
         }
     }
@@ -1027,6 +1072,9 @@ fn emit_iupac_warning_write_once() {
 /// One-time disclosure that a FASTQ header description was not carried into uBAM
 /// output. Echoes the text actually dropped, because what is lost varies: an
 /// instrument identifier, or an Illumina `1:N:0:INDEX` field.
+///
+/// "FASTQ" is unconditional: BAM input cannot reach this, because a whitespace QNAME is
+/// refused and the tab introducing a tag tail always precedes any space in a tag value.
 fn emit_description_dropped_once(dropped: &str) {
     static SEEN: OnceLock<()> = OnceLock::new();
     SEEN.get_or_init(|| {
@@ -1044,8 +1092,12 @@ fn emit_description_dropped_once(dropped: &str) {
 }
 
 /// Format `{TYPE}:{VALUE}` for one BAM aux field, matching `samtools fastq -T`.
+///
+/// Every arm's output is scanned before returning, so no type code can route text into the
+/// FASTQ id unchecked.
 fn append_tag_type_and_value(out: &mut String, value: &Value<'_>) -> Result<()> {
     use std::fmt::Write;
+    let start = out.len();
     match value {
         Value::Character(c) => write!(out, "A:{}", *c as char).unwrap(),
         Value::Int8(v) => write!(out, "i:{}", v).unwrap(),
@@ -1080,7 +1132,7 @@ fn append_tag_type_and_value(out: &mut String, value: &Value<'_>) -> Result<()>
             );
         }
     }
-    Ok(())
+    reject_framing_in_tag_value(&out[start..])
 }
 
 #[cfg(test)]
@@ -1406,6 +1458,110 @@ mod tests {
         );
     }
 
+    #[test]
+    fn qname_boundary_bytes_rejected() {
+        for (label, name) in [
+            ("space", b"read one".as_slice()),
+            ("tab", b"read\tone"),
+            ("LF", b"read\none"),
+            ("CR", b"read\rone"),
+            ("FF", b"read\x0Cone"),
+        ] {
+            assert!(
+                reject_whitespace_in_qname(name).is_err(),
+                "{} must be rejected in a QNAME",
+                label
+            );
+        }
+    }
+
+    #[test]
+    fn qname_non_boundary_bytes_accepted() {
+        // VT is accepted by decision, not by derivation: widening the set here would
+        // over-reject files that work today.
+        for (label, name) in [
+            ("plain", b"read_one".as_slice()),
+            ("VT", b"read\x0Bone"),
+            ("at-sign", b"read@one"),
+            ("non-ASCII", "read\u{e9}one".as_bytes()),
+        ] {
+            assert!(
+                reject_whitespace_in_qname(name).is_ok(),
+                "{} must be accepted in a QNAME",
+                label
+            );
+        }
+    }
+
+    #[test]
+    fn qname_error_escapes_invisible_bytes() {
+        let err = reject_whitespace_in_qname(b"read\tone").expect_err("tab must be rejected");
+        let msg = format!("{:#}", err);
+        assert!(
+            msg.contains("read\\tone"),
+            "error must show the offending byte escaped, got: {}",
+            msg
+        );
+    }
+
+    #[test]
+    fn tag_value_framing_bytes_rejected() {
+        for (label, value) in [
+            ("tab", "AAA\tCCC"),
+            ("LF", "AAA\nCCC"),
+            ("CR", "AAA\rCCC"),
+            ("FF", "AAA\x0CCCC"),
+        ] {
+            assert!(
+                reject_framing_in_tag_value(value).is_err(),
+                "{} must be rejected in a Z tag value",
+                label
+            );
+        }
+    }
+
+    #[test]
+    fn tag_value_space_accepted() {
+        // `Z` is `[ !-~]*`, so a space is legal — and the tab introducing the tag
+        // precedes it, leaving `parse_name_and_data` able to split the tail.
+        assert!(reject_framing_in_tag_value("has a space").is_ok());
+    }
+
+    #[test]
+    fn read_side_emitter_rejects_newline_in_character_tag() {
+        // `A` writes its raw byte into the same id as `Z`, so it needs the same guard.
+        let value = Value::Character(b'\n');
+        let mut buf = String::new();
+        let err = append_tag_type_and_value(&mut buf, &value)
+            .expect_err("a newline in an A value must be rejected by the read-side emitter");
+        assert!(
+            format!("{:#}", err).contains("cannot pass through the FASTQ header"),
+            "expected the framing rejection, got: {:#}",
+            err
+        );
+    }
+
+    #[test]
+    fn read_side_emitter_accepts_printable_character_tag() {
+        let mut buf = String::new();
+        append_tag_type_and_value(&mut buf, &Value::Character(b'+')).unwrap();
+        assert_eq!(buf, "A:+");
+    }
+
+    #[test]
+    fn read_side_emitter_rejects_newline_in_string_tag() {
+        let value = Value::String(bstr::BStr::new(b"AAA\nCCC"));
+        let mut buf = String::new();
+        let err = append_tag_type_and_value(&mut buf, &value)
+            .expect_err("a newline in a Z value must be rejected by the read-side emitter");
+        let msg = format!("{:#}", err);
+        assert!(
+            msg.contains("AAA\\nCCC"),
+            "expected the escaped value in the message, got: {}",
+            msg
+        );
+    }
+
     #[test]
     fn parse_unknown_type_code_rejected() {
         let err = parse_name_and_data("@read1\tXX:Q:foo").unwrap_err();