Skip to content

Check the CRCs of a BAM's header blocks when VALIDATE_CRC_CHECKSUMS is on #1953

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

Open
tfenne wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Check the CRCs of a BAM's header blocks when VALIDATE_CRC_CHECKSUMS is on #1953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ early infrastructure for a plugin-based codec framework and resource bundles.

---

## 6.0.1

Patch release on the 6.0 line.

### Bug fixes

- **`SamReaderFactory.Option.VALIDATE_CRC_CHECKSUMS` checks the blocks that hold a BAM's header** ([#1949](https://github.com/samtools/htsjdk/issues/1949)). The option reached the reader only after it had read the header as it opened, so a header block with a wrong CRC, and any records sharing it, went unchecked.

---

## 6.0.0

Major release. It includes every change in 5.0.2. Before upgrading, read [Upgrading from 5.x](#upgrading-from-5x); to explain differences from files 5.x wrote, see [Output that differs from 5.x](#output-that-differs-from-5x).
Expand Down
57 changes: 38 additions & 19 deletions src/main/java/htsjdk/samtools/BAMFileReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
useAsynchronousIO,
validationStringency,
samRecordFactory,
BlockGunzipper.getDefaultInflaterFactory());
BlockGunzipper.getDefaultInflaterFactory(),
false);
}

/**
Expand All @@ -126,6 +127,7 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
* @param validationStringency Controls how to handle invalidate reads or header lines.
* @param samRecordFactory SAM record factory
* @param inflaterFactory InflaterFactory used by BlockCompressedInputStream
* @param checkCrcs if true, check the CRC32 of every block, including those of the header, which is read here
* @throws IOException
*/
BAMFileReader(
Expand All @@ -135,13 +137,15 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
final boolean useAsynchronousIO,
final ValidationStringency validationStringency,
final SAMRecordFactory samRecordFactory,
final InflaterFactory inflaterFactory)
final InflaterFactory inflaterFactory,
final boolean checkCrcs)
throws IOException {
mIndexPath = indexPath;
mIsSeekable = false;
mCompressedInputStream = useAsynchronousIO
? new AsyncBlockCompressedInputStream(stream, inflaterFactory)
: new BlockCompressedInputStream(stream, inflaterFactory);
mCompressedInputStream.setCheckCrcs(checkCrcs);
mStream = new BinaryCodec(new DataInputStream(mCompressedInputStream));
this.eagerDecode = eagerDecode;
this.mValidationStringency = validationStringency;
Expand Down Expand Up @@ -174,7 +178,8 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
useAsynchronousIO,
validationStringency,
samRecordFactory,
BlockGunzipper.getDefaultInflaterFactory());
BlockGunzipper.getDefaultInflaterFactory(),
false);
}

/**
Expand All @@ -186,6 +191,7 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
* @param validationStringency Controls how to handle invalidate reads or header lines.
* @param samRecordFactory SAM record factory
* @param inflaterFactory InflaterFactory used by BlockCompressedInputStream
* @param checkCrcs if true, check the CRC32 of every block, including those of the header, which is read here
* @throws IOException
*/
BAMFileReader(
Expand All @@ -195,14 +201,12 @@ public class BAMFileReader extends SamReader.ReaderImplementation {
final boolean useAsynchronousIO,
final ValidationStringency validationStringency,
final SAMRecordFactory samRecordFactory,
final InflaterFactory inflaterFactory)
final InflaterFactory inflaterFactory,
final boolean checkCrcs)
throws IOException {
this(
useAsynchronousIO
? new AsyncBlockCompressedInputStream(
new SeekablePathStream(checkPathExists(path)), inflaterFactory)
: new BlockCompressedInputStream(
new SeekablePathStream(checkPathExists(path)), inflaterFactory),
openBlockCompressed(
new SeekablePathStream(checkPathExists(path)), useAsynchronousIO, inflaterFactory, checkCrcs),
indexPath != null ? indexPath : SamFiles.findIndex(path),
eagerDecode,
useAsynchronousIO,
Expand Down Expand Up @@ -245,6 +249,19 @@ private static Path checkPathExists(final Path path) throws IOException {
return path;
}

/** Opens a block-compressed stream over {@code strm}, set to check CRCs before anything is read from it. */
private static BlockCompressedInputStream openBlockCompressed(
final SeekableStream strm,
final boolean useAsynchronousIO,
final InflaterFactory inflaterFactory,
final boolean checkCrcs) {
final BlockCompressedInputStream stream = useAsynchronousIO
? new AsyncBlockCompressedInputStream(strm, inflaterFactory)
: new BlockCompressedInputStream(strm, inflaterFactory);
stream.setCheckCrcs(checkCrcs);
return stream;
}

/**
* Prepare to read BAM from a stream (seekable)
* @param strm source of bytes
Expand All @@ -270,7 +287,8 @@ private static Path checkPathExists(final Path path) throws IOException {
useAsynchronousIO,
validationStringency,
samRecordFactory,
BlockGunzipper.getDefaultInflaterFactory());
BlockGunzipper.getDefaultInflaterFactory(),
false);
}

/**
Expand All @@ -282,6 +300,7 @@ private static Path checkPathExists(final Path path) throws IOException {
* @param validationStringency Controls how to handle invalidate reads or header lines.
* @param samRecordFactory SAM record factory
* @param inflaterFactory InflaterFactory used by BlockCompressedInputStream
* @param checkCrcs if true, check the CRC32 of every block, including those of the header, which is read here
* @throws IOException
*/
BAMFileReader(
Expand All @@ -291,12 +310,11 @@ private static Path checkPathExists(final Path path) throws IOException {
final boolean useAsynchronousIO,
final ValidationStringency validationStringency,
final SAMRecordFactory samRecordFactory,
final InflaterFactory inflaterFactory)
final InflaterFactory inflaterFactory,
final boolean checkCrcs)
throws IOException {
this(
useAsynchronousIO
? new AsyncBlockCompressedInputStream(strm, inflaterFactory)
: new BlockCompressedInputStream(strm, inflaterFactory),
openBlockCompressed(strm, useAsynchronousIO, inflaterFactory, checkCrcs),
indexPath,
eagerDecode,
useAsynchronousIO,
Expand Down Expand Up @@ -330,7 +348,8 @@ private static Path checkPathExists(final Path path) throws IOException {
useAsynchronousIO,
validationStringency,
samRecordFactory,
BlockGunzipper.getDefaultInflaterFactory());
BlockGunzipper.getDefaultInflaterFactory(),
false);
}

/**
Expand All @@ -342,6 +361,7 @@ private static Path checkPathExists(final Path path) throws IOException {
* @param validationStringency Controls how to handle invalidate reads or header lines.
* @param samRecordFactory SAM record factory
* @param inflaterFactory InflaterFactory used by BlockCompressedInputStream
* @param checkCrcs if true, check the CRC32 of every block, including those of the header, which is read here
* @throws IOException
*/
BAMFileReader(
Expand All @@ -351,12 +371,11 @@ private static Path checkPathExists(final Path path) throws IOException {
final boolean useAsynchronousIO,
final ValidationStringency validationStringency,
final SAMRecordFactory samRecordFactory,
final InflaterFactory inflaterFactory)
final InflaterFactory inflaterFactory,
final boolean checkCrcs)
throws IOException {
this(
useAsynchronousIO
? new AsyncBlockCompressedInputStream(strm, inflaterFactory)
: new BlockCompressedInputStream(strm, inflaterFactory),
openBlockCompressed(strm, useAsynchronousIO, inflaterFactory, checkCrcs),
indexStream,
eagerDecode,
useAsynchronousIO,
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/htsjdk/samtools/SamReaderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,9 @@ public SamReader open(final SamInputResource resource) {
final InputResource data = resource.data();
final InputResource indexMaybe = resource.indexMaybe();
final boolean indexDefined = indexMaybe != null;
// Given to a BAM reader as it is made, rather than applied with the other options below, because the
// reader reads the header as it is made
final boolean checkCrcs = enabledOptions.contains(Option.VALIDATE_CRC_CHECKSUMS);

final InputResource.Type type = data.type();
if (type == InputResource.Type.URL) {
Expand Down Expand Up @@ -406,7 +409,8 @@ public SamReader open(final SamInputResource resource) {
asynchronousIO,
validationStringency,
this.samRecordFactory,
this.inflaterFactory);
this.inflaterFactory,
checkCrcs);
} else if (SamStreams.sourceLikeCram(data.asUnbufferedSeekableStream())) {
if (referenceSource == null) {
referenceSource = ReferenceSource.getDefaultCRAMReferenceSource();
Expand Down Expand Up @@ -452,7 +456,8 @@ public SamReader open(final SamInputResource resource) {
asynchronousIO,
validationStringency,
this.samRecordFactory,
this.inflaterFactory);
this.inflaterFactory,
checkCrcs);
} else {
// seekable.
// need to return to the beginning because it's the same stream we used earlier
Expand All @@ -465,7 +470,8 @@ public SamReader open(final SamInputResource resource) {
asynchronousIO,
validationStringency,
this.samRecordFactory,
this.inflaterFactory);
this.inflaterFactory,
checkCrcs);
}
} else {
bufferedStream.close();
Expand All @@ -476,7 +482,8 @@ public SamReader open(final SamInputResource resource) {
asynchronousIO,
validationStringency,
this.samRecordFactory,
this.inflaterFactory);
this.inflaterFactory,
checkCrcs);
}
} else if (BlockCompressedInputStream.isValidFile(bufferedStream)) {
// Pass the configured inflater factory through so that BGZF-compressed
Expand Down
41 changes: 41 additions & 0 deletions src/test/java/htsjdk/samtools/SamReaderFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import htsjdk.testutil.ftp.LocalFtpServer;
import htsjdk.testutil.http.LocalHttpServer;
import htsjdk.testutil.streams.SeekableByteChannelFromBuffer;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
Expand Down Expand Up @@ -713,4 +715,43 @@ public void testWriteAndReadFromPipe()
}
}
}

// VALIDATE_CRC_CHECKSUMS must cover the blocks that hold the header, which a BAM reader reads as it opens.

/** A small BAM with the CRC of its first block, which holds the header, made wrong. */
private static byte[] bamWithCorruptHeaderBlockCrc() {
final SAMRecordSetBuilder records = new SAMRecordSetBuilder();
records.addFrag("r1", 0, 100, false);
final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
try (SAMFileWriter writer = new SAMFileWriterFactory().makeBAMWriter(records.getHeader(), false, bytes)) {
records.forEach(writer::addAlignment);
}
final byte[] bam = bytes.toByteArray();
final int bsizeOffset = BlockCompressedStreamConstants.BLOCK_LENGTH_OFFSET;
final int firstBlockLength = ((bam[bsizeOffset] & 0xFF) | (bam[bsizeOffset + 1] & 0xFF) << 8) + 1;
bam[firstBlockLength - 8] ^= 0xFF;
return bam;
}

@Test
public void testCrcValidationCoversTheHeaderOfABamStream() {
final byte[] bam = bamWithCorruptHeaderBlockCrc();
final SamReaderFactory factory =
SamReaderFactory.makeDefault().enable(SamReaderFactory.Option.VALIDATE_CRC_CHECKSUMS);
Assert.assertThrows(
SAMFormatException.class, () -> factory.open(SamInputResource.of(new ByteArrayInputStream(bam))));
}

@Test
public void testCrcValidationCoversTheHeaderOfABamFile() throws IOException {
final Path bamPath = Files.createTempFile("corrupt_header_crc", ".bam");
try {
Files.write(bamPath, bamWithCorruptHeaderBlockCrc());
final SamReaderFactory factory =
SamReaderFactory.makeDefault().enable(SamReaderFactory.Option.VALIDATE_CRC_CHECKSUMS);
Assert.assertThrows(SAMFormatException.class, () -> factory.open(bamPath));
} finally {
Files.deleteIfExists(bamPath);
}
}
}
Loading