Warn if bgzf_getline() returned apparently UTF-16-encoded text by jmarshall · Pull Request #1487 · samtools/htslib

jmarshall · 2022-08-01T09:05:29Z

Text files badly transferred from Windows may occasionally be UTF-16-encoded, and this may not be easily noticed by the user — see for example, this report of tabix woes (??# is as reproduced locally by me, and equally as confusing as the OP's output):

$ tabix myVcf.vcf.gz
[E::get_intv] Failed to parse TBX_VCF, was wrong -p [type] used?
The offending line was: "??#"
[E::get_intv] Failed to parse TBX_VCF, was wrong -p [type] used?
The offending line was: ""
[E::get_intv] Failed to parse TBX_VCF, was wrong -p [type] used?
The offending line was: ""

It turned out that the text VCF file was UTF-16-encoded, and the lines returned contained alternating NULs when interpreted as ASCII C-strings, hence all lines appearing truncated to 0 or 1 or BOM+1 characters.

HTSlib should not accept such encoding (as other tools surely don't, hence doing so would cause interoperability problems), but it should ideally emit a warning or error message identifying the problem clearly.

Reading text from a htsFile/samFile/vcfFile will already have failed with EFTYPE/ENOEXEC (and printed a suitable Inappropriate file type or format message) if the text file is UTF-16-encoded, as the encoding will not have been recognised by hts_detect_format(). So no changes are needed here, although this could be extended to add utf16_text_format or so to htsFormatCategory to aid in reporting this (but I don't think that is really warranted).

OTOH code that uses plain BGZF handles does not return a clear diagnostic, as was seen with tabix. bgzf_getline() will return a UTF-16-encoded text line successfully, but code not expecting it will misinterpret the resulting string. This PR adds a diagnostic suitable for each context to the BGZF-based bgzf_getline() calls in HTSlib:

in hts_readlist()/hts_readlines(), emit a warning (once, on the first line);
in tbx.c, emit a more specific error message if get_intv() parsing failure is due to UTF-16 encoding.

With this PR, the biostars poster's test case produces the following diagnostics and the root cause is apparent:

tabix myVcf.vcf.gz
[E::get_intv] Failed to parse TBX_VCF: offending line appears to be encoded as UTF-16
[E::get_intv] Failed to parse TBX_VCF: offending line appears to be encoded as UTF-16
[E::get_intv] Failed to parse TBX_VCF: was wrong -p [type] used?
The offending line was: ""

d-cameron · 2022-08-01T11:03:23Z

As of VCF 4.3 encoding is explicit:

The character encoding of VCF files is UTF-8.

Newlines are also explicit:

Line separators must be CR+LF or LF

jmarshall · 2022-08-01T11:15:30Z

The data file in the OP's case was a VCF file, and it is true that VCF 4.3 explicitly and VCF 4.1/4.2 de facto require UTF-8 encoding. However tabix also works with arbitrary position-oriented files that don't have specifications about encodings.

However there is no suggestion (in this PR, at least) that HTSlib or tabix should go out of their way to accept such wackily encoded input files. This PR just improves the error messages so that the user is aided in identifying the underlying cause of the parsing failure.

daviesrob · 2022-08-01T11:31:50Z

This looks useful, but I'm not sure it quite works as intended yet. If I make myself a UTF-16 encoded vcf and try to index it, I get:

$ iconv -t UTF-16 test/index.vcf | ./bgzip > /tmp/i16.vcf.gz
$ ./tabix /tmp/i16.vcf.gz
[E::get_intv] Failed to parse TBX_VCF: offending line appears to be encoded as UTF-16
[E::get_intv] Failed to parse TBX_VCF: offending line appears to be encoded as UTF-16
  [ ... lots of similar messages omitted ...]
[E::get_intv] Failed to parse TBX_VCF: offending line appears to be encoded as UTF-16
[E::hts_idx_push] Unsorted positions on sequence #1: 2 followed by 1
tbx_index_build failed: /tmp/i16.vcf.gz

So it's reporting once for each line, because the indexer skips lines it can't parse. I guess that isn't much change from previous behaviour which also printed a message for each bad line, but should we treat this as more fatal? If the whole file is UTF-16 then there's no way you're going to get an index for it.

Somewhat more interesting is the last line. That appeared because tbx_parse1() gets to look at the line before any UTF-16 check happens. It will also happily split input lines on NUL bytes, so occasionally it interprets what it gets as VCF even though the returned data is rubbish. Possibly it should reject lines that appear to have premature NUL bytes so the rest of the detection can work more reliably?

jkbonfield · 2022-08-01T11:38:34Z

I'd be +1 for making it a fatal error instead.

Although as for warnings, I'm quite OK with invalid input producing 1 warning per line for invalid data. It "encourages" people to fix their inputs and makes spotting the warning significantly more likely. :-) (Obviously warnings produced from valid data, eg something we don't fully support or another form of warning, shouldn't be quite so spammy.)

jmarshall · 2022-08-01T11:39:19Z

Yes. In general I prefer not to add diagnostic output to library routines, so in the get_intv() case I just changed (in some cases) the existing message being printed rather than altered when a diagnostic might be printed. So it's working as intended by the PR's author 😄 but I suppose I could be convinced to make more drastic changes to get_intv()'s / tbx_parse1()'s behaviour. (Is it the right time for contemplating further behaviour changes?)

daviesrob · 2022-08-01T14:04:29Z

Well, as no files supported by tabix should have NUL bytes in them, I don't think it's too unreasonable to have tbx_parse1() reject lines that look obviously wrong. It would be pretty easy to make it look for something like len > 6, a NUL is found in the first four bytes and maybe also there's another NUL two bytes on. That should make it better at catching the UTF-16 case without getting false positives.

Also, as hts_is_utf16_text() is not going to mistake something valid for UTF-16, I think it would be reasonable to make get_intv() return -2 if it triggers, turning the error into a fatal one. Otherwise you could end up with the program apparently working, albeit very noisily, but making an empty index - which actually gets more likely if tbx_parse1() is better at detecting bad data.

Text files badly transferred from Windows may occasionally be UTF-16-encoded, and this may not be easily noticed by the user. HTSlib should not accept such encoding (as other tools surely don't, hence doing so would cause interoperability problems), but it should ideally emit a warning or error message identifying the problem. Reading text from a htsFile/samFile/vcfFile will already have failed with EFTYPE/ENOEXEC if the text file is UTF-16-encoded, as the encoding will not have been recognised by hts_detect_format(). OTOH bgzf_getline() will return a UTF-16-encoded text line. Add a suitable context-dependent diagnostic to the BGZF-based bgzf_getline() calls in HTSlib: in hts_readlist()/hts_readlines(), emit a warning (once, on the first line); in tbx.c, emit a more specific error message if get_intv() parsing failure is due to UTF-16 encoding. [TODO] If utf16_text_format were added to htsFormatCategory, the new is_utf16_text() function is suitable for detecting it.

jmarshall · 2024-05-23T09:25:14Z

Oh dear… I should have mentioned that in the last couple of weeks I've been in the process of reworking this to address your comments in #1487 (comment) and add a specific warning about UTF-8-encoded BOMs as well.

Oh well, thanks for merging 😄 and I'll look at reworking those changes as a followup PR…

daviesrob · 2024-05-23T11:37:36Z

I made a note to myself to make tabix reject this sort of input properly. But I'm happy to wait if you have something brewing.

Notice: this is the last SAMtools / HTSlib release where CRAM 3.0 will be the default CRAM version. From the next we will change to CRAM 3.1 unless the version is explicitly specified, for example using "samtools view -O cram,version=3.0". Updates ------- * Extend annot-tsv with several new command line options. --delim permits use of other delimiters. --headers for selection of other header formats. --no-header-idx to suppress column index numbers in header. Also removed -h as it is now short for --headers. Note --help still works. (PR samtools#1779) * Allow annot-tsv -a to rename annotations. (PR samtools#1709) * Extend annot-tsv --overlap to be able to specify the overlap fraction separately for source and target. (PR samtools#1811) * Added new APIs to facilitate low-level CRAM container manipulations, used by the new "samtools cat" region filtering code. Functions are: cram_container_get_coords() cram_filter_container() cram_index_extents() cram_container_num2offset() cram_container_offset2num() cram_num_containers() cram_num_containers_between() Also improved cram_index_query() to cope with HTS_IDX_NOCOOR regions. (PR samtools#1771) * Bgzip now retains file modification and access times when compressing and decompressing. (PR samtools#1727, fixes samtools#1718. Requested by Gert Hulselmans.) * Use FNV1a for string hashing in khash. The old algorithm was particularly weak with base-64 style strings and lead to a large number of collisions. (PR samtools#1806. Fixes samtools/samtools#2066, reported by Hans-Joachim Ruscheweyh) * Improve the speed of the nibble2base() function on Intel (PR samtools#1667, PR samtools#1764, PR samtools#1786, PR samtools#1802, thanks to Ruben Vorderman) and ARM (PR samtools#1795, thanks to John Marshall). * bgzf_getline() will now warn if it encounters UTF-16 data. (PR samtools#1487, thanks to John Marshall) * Speed up bgzf_read(). While this does not reduce CPU significantly, it does increase the maximum parallelism available permitting 10-15% faster decoding. (PR samtools#1772, PR samtools#1800, Issue samtools#1798) * Speed up faidx by use of better isgraph methods (PR samtools#1797) and whole-line reading (PR samtools#1799, thanks to John Marshall). * Speed up kputll() function, speeding up BAM -> SAM conversion by about 5% and also samtools depth. (PR samtools#1805) * Added more example code, covering fasta/fastq indexing, tabix indexing and use of the thread pool. (PR samtools#1666) Build Changes ------------- * Code warning fixes for pedantic compilers (PR samtools#1777) and avoid some undefined behaviour (PR samtools#1810, PR samtools#1816, PR samtools#1828). * Windows based CI has been migrated from AppVeyor to GitHub Actions. (PR samtools#1796, PR samtools#1803, PR samtools#1808) * Miscellaneous minor build infrastructure and code fixes. (PR samtools#1807, PR samtools#1829, both thanks to John Marshall) * Updated htscodecs submodule to version 1.6.1 (PR samtools#1828) * Fixed an awk script in the Makefile that only worked with gawk. (PR samtools#1831) Bug fixes --------- * Fix small OSS-Fuzz reported issues with CRAM encoding and long CIGARS and/or illegal positions. (PR samtools#1775, PR samtools#1801, PR samtools#1817) * Fix issues with on-the-fly indexing of VCF/BCF (bcftools --write-index) when not using multiple threads. (PR samtools#1837. Fixes samtools/bcftools#2267, reported by Giulio Genovese) * Stricter limits on POS / MPOS / TLEN in sam_parse1(). This fixes a signed overflow reported by OSS-Fuzz and should help prevent other as-yet undetected bugs. (PR samtools#1812) * Check that the underlying file open worked for preload: URLs. Fixes a NULL pointer dereference reported by OSS-Fuzz. (PR samtools#1821) * Fix an infinite loop in hts_itr_query() when given extremely large positions which cause integer overflow. Also adds hts_bin_maxpos() and hts_idx_maxpos() functions. (PR samtools#1774, thanks to John Marshall and reported by Jesus Alberto Munoz Mesa) * Fix an out of bounds read in hts_itr_multi_next() when switching chromosomes. This bug is present in releases 1.11 to 1.20. (PR samtools#1788. Fixes samtools/samtools#2063, reported by acorvelo) * Work around parsing problems with colons in CHROM names. Fixes samtools/bcftools#2139. (PR samtools#1781, John Marshall / James Bonfield) * Correct the CPU detection for Mac OS X 10.7. cpuid is used by htscodecs (see samtools/htscodecs#116), and the corresponding changes in htslib are PR samtools#1785. Reported by Ryan Carsten Schmidt. * Make BAM zero-length intervals work the same as CRAM; permitted and returning overlapping records. (PR samtools#1787. Fixes samtools/samtools#2060, reported by acorvelo) * Replace assert() with abort() in BCF synced reader. This is not an ideal solution, but it gives consistent behaviour when compiling with or without NDEBUG. (PR samtools#1791, thanks to Martin Pollard) * Fixed failure to change the write block size on compressed SAM or VCF files due to an internal type confusion. (PR samtools#1826) * Fixed an out-of-bounds read in cram_codec_iter_next() (PR samtools#1832)

htslib release 1.21: The primary user-visible changes in this release are updates to the annot-tsv tool and some speed improvements. Full details of other changes and bugs fixed are below. Notice: this is the last SAMtools / HTSlib release where CRAM 3.0 will be the default CRAM version. From the next we will change to CRAM 3.1 unless the version is explicitly specified, for example using "samtools view -O cram,version=3.0". Updates ------- * Extend annot-tsv with several new command line options. --delim permits use of other delimiters. --headers for selection of other header formats. --no-header-idx to suppress column index numbers in header. Also removed -h as it is now short for --headers. Note --help still works. (PR samtools#1779) * Allow annot-tsv -a to rename annotations. (PR samtools#1709) * Extend annot-tsv --overlap to be able to specify the overlap fraction separately for source and target. (PR samtools#1811) * Added new APIs to facilitate low-level CRAM container manipulations, used by the new "samtools cat" region filtering code. Functions are: cram_container_get_coords() cram_filter_container() cram_index_extents() cram_container_num2offset() cram_container_offset2num() cram_num_containers() cram_num_containers_between() Also improved cram_index_query() to cope with HTS_IDX_NOCOOR regions. (PR samtools#1771) * Bgzip now retains file modification and access times when compressing and decompressing. (PR samtools#1727, fixes samtools#1718. Requested by Gert Hulselmans.) * Use FNV1a for string hashing in khash. The old algorithm was particularly weak with base-64 style strings and lead to a large number of collisions. (PR samtools#1806. Fixes samtools/samtools#2066, reported by Hans-Joachim Ruscheweyh) * Improve the speed of the nibble2base() function on Intel (PR samtools#1667, PR samtools#1764, PR samtools#1786, PR samtools#1802, thanks to Ruben Vorderman) and ARM (PR samtools#1795, thanks to John Marshall). * bgzf_getline() will now warn if it encounters UTF-16 data. (PR samtools#1487, thanks to John Marshall) * Speed up bgzf_read(). While this does not reduce CPU significantly, it does increase the maximum parallelism available permitting 10-15% faster decoding. (PR samtools#1772, PR samtools#1800, Issue samtools#1798) * Speed up faidx by use of better isgraph methods (PR samtools#1797) and whole-line reading (PR samtools#1799, thanks to John Marshall). * Speed up kputll() function, speeding up BAM -> SAM conversion by about 5% and also samtools depth. (PR samtools#1805) * Added more example code, covering fasta/fastq indexing, tabix indexing and use of the thread pool. (PR samtools#1666) Build Changes ------------- * Code warning fixes for pedantic compilers (PR samtools#1777) and avoid some undefined behaviour (PR samtools#1810, PR samtools#1816, PR samtools#1828). * Windows based CI has been migrated from AppVeyor to GitHub Actions. (PR samtools#1796, PR samtools#1803, PR samtools#1808) * Miscellaneous minor build infrastructure and code fixes. (PR samtools#1807, PR samtools#1829, both thanks to John Marshall) * Updated htscodecs submodule to version 1.6.1 (PR samtools#1828) * Fixed an awk script in the Makefile that only worked with gawk. (PR samtools#1831) Bug fixes --------- * Fix small OSS-Fuzz reported issues with CRAM encoding and long CIGARS and/or illegal positions. (PR samtools#1775, PR samtools#1801, PR samtools#1817) * Fix issues with on-the-fly indexing of VCF/BCF (bcftools --write-index) when not using multiple threads. (PR samtools#1837. Fixes samtools/bcftools#2267, reported by Giulio Genovese) * Stricter limits on POS / MPOS / TLEN in sam_parse1(). This fixes a signed overflow reported by OSS-Fuzz and should help prevent other as-yet undetected bugs. (PR samtools#1812) * Check that the underlying file open worked for preload: URLs. Fixes a NULL pointer dereference reported by OSS-Fuzz. (PR samtools#1821) * Fix an infinite loop in hts_itr_query() when given extremely large positions which cause integer overflow. Also adds hts_bin_maxpos() and hts_idx_maxpos() functions. (PR samtools#1774, thanks to John Marshall and reported by Jesus Alberto Munoz Mesa) * Fix an out of bounds read in hts_itr_multi_next() when switching chromosomes. This bug is present in releases 1.11 to 1.20. (PR samtools#1788. Fixes samtools/samtools#2063, reported by acorvelo) * Work around parsing problems with colons in CHROM names. Fixes samtools/bcftools#2139. (PR samtools#1781, John Marshall / James Bonfield) * Correct the CPU detection for Mac OS X 10.7. cpuid is used by htscodecs (see samtools/htscodecs#116), and the corresponding changes in htslib are PR samtools#1785. Reported by Ryan Carsten Schmidt. * Make BAM zero-length intervals work the same as CRAM; permitted and returning overlapping records. (PR samtools#1787. Fixes samtools/samtools#2060, reported by acorvelo) * Replace assert() with abort() in BCF synced reader. This is not an ideal solution, but it gives consistent behaviour when compiling with or without NDEBUG. (PR samtools#1791, thanks to Martin Pollard) * Fixed failure to change the write block size on compressed SAM or VCF files due to an internal type confusion. (PR samtools#1826) * Fixed an out-of-bounds read in cram_codec_iter_next() (PR samtools#1832) # -----BEGIN PGP SIGNATURE----- # # iQJJBAABCgAzFiEEaGn1wQ1nqxSLDC2XHsvluhXGTMwFAmbjBlMVHHJtZCtnaXRA # c2FuZ2VyLmFjLnVrAAoJEB7L5boVxkzMgb0P/3AAxkJNLGK8Q1pBBo+in7KzwBOc # /nRfvReki47r4+i1ev98abEF/+XZoYSS8ZkiJH8JMkLCclb1ch2fDQTIL/sjJwrU # MfhYx6oHhzLUOjpG24m390wlvlkEsg5S69jbgImWxLQ1n4ZcSuZGumiwPx5274w5 # 3R/K9vIXtuSF9DTD/jrTgoZeBiN+B4XefAJqHLSo4/XzDo7DNMSZryNGFpHdyKcB # oEfEXX45vJLgzA3Fxo3LqNpC8UTDO5C9u9Uup+hCLN7RTBjV5Fu0UxcwWTSPqfiC # Jroi2FhxNPlDkRL3jraI8LrB3Cqb0G/31OU+cryasrqYnfJrl0e/MyXiRSa0sg25 # uHHWVyyC4JR7uBfUCppTVf76iajbkIVpHgHLZS1heGwM4PCL+AalvsIgGcvLXcBs # g/AwZLoXzao+pbzg8hzN1JuLB5hHcrHp6TsQ6JbTTrWaLeAL3s/+eQ4+d+Np2iUn # v8TMg6onu1HiNgVMQcFXSLC2Q51rB+lD6AA3eusKjcQsaxyk+U7SLCx3OhbvxGsd # SjxQWP1W+WW//e9nm5f4CTSSf1PHoDMyR/kVEc3qwEWJslri68RyB69HrDIVzrWT # 3gKAKJYafi5ssFHsKTsCngESpDsTg3aQV1AGOQPLN/TIjyYDTUOL+/HylRs3k9XN # KOGO5yggx+yqeL3w # =lHVl # -----END PGP SIGNATURE----- # gpg: Signature made Thu 12 Sep 2024 08:18:43 AM PDT # gpg: using RSA key 6869F5C10D67AB148B0C2D971ECBE5BA15C64CCC # gpg: issuer "rmd+git@sanger.ac.uk" # gpg: Can't check signature: No public key

daviesrob self-assigned this Aug 30, 2022

jmarshall force-pushed the utf16 branch from c8a062b to 714fc63 Compare May 12, 2024 00:24

daviesrob merged commit 30c9c50 into samtools:develop May 23, 2024

jmarshall deleted the utf16 branch May 23, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Warn if bgzf_getline() returned apparently UTF-16-encoded text#1487

Warn if bgzf_getline() returned apparently UTF-16-encoded text#1487
daviesrob merged 1 commit into
samtools:developfrom
jmarshall:utf16

jmarshall commented Aug 1, 2022 •

edited

Loading

Uh oh!

d-cameron commented Aug 1, 2022 •

edited

Loading

Uh oh!

jmarshall commented Aug 1, 2022

Uh oh!

daviesrob commented Aug 1, 2022

Uh oh!

jkbonfield commented Aug 1, 2022

Uh oh!

jmarshall commented Aug 1, 2022 •

edited

Loading

Uh oh!

daviesrob commented Aug 1, 2022

Uh oh!

jmarshall commented May 23, 2024

Uh oh!

daviesrob commented May 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jmarshall commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-cameron commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarshall commented Aug 1, 2022

Uh oh!

daviesrob commented Aug 1, 2022

Uh oh!

jkbonfield commented Aug 1, 2022

Uh oh!

jmarshall commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daviesrob commented Aug 1, 2022

Uh oh!

jmarshall commented May 23, 2024

Uh oh!

daviesrob commented May 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarshall commented Aug 1, 2022 •

edited

Loading

d-cameron commented Aug 1, 2022 •

edited

Loading

jmarshall commented Aug 1, 2022 •

edited

Loading