From fff5994a8f70ea6fac0f78e6e7bd3b4c4be7d023 Mon Sep 17 00:00:00 2001 From: Roberto Rossini <71787608+robomics@users.noreply.github.com> Date: Tue, 16 Apr 2024 11:55:50 +0200 Subject: [PATCH 1/4] Bugfix Fix out of bound access that could occur when satisfying asymmetric cis queries partially overlapping with the matrix diagonal. --- src/hictkr_file.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hictkr_file.cpp b/src/hictkr_file.cpp index b7577b7..b62b6fc 100644 --- a/src/hictkr_file.cpp +++ b/src/hictkr_file.cpp @@ -261,7 +261,10 @@ static Rcpp::NumericMatrix fetch_as_matrix(const Selector &sel) { } else if (i2 - i1 > num_cols && i1 < num_cols && i2 < num_rows) { const auto i3 = static_cast(tp.bin2_id - row_offset); const auto i4 = static_cast(tp.bin1_id - col_offset); - matrix.at(i3, i4) = tp.count; + + if (i3 >= 0 && i3 < num_rows && i4 >= 0 && i4 < num_cols) { + matrix.at(i3, i4) = tp.count; + } } } }); From b6a0767478fe3e787a6a6fdaffaa9091e440998d Mon Sep 17 00:00:00 2001 From: Roberto Rossini <71787608+robomics@users.noreply.github.com> Date: Tue, 16 Apr 2024 11:59:20 +0200 Subject: [PATCH 2/4] Add more tests --- tests/testthat/test-fetch-dense.R | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-fetch-dense.R b/tests/testthat/test-fetch-dense.R index c00e303..b2df602 100644 --- a/tests/testthat/test-fetch-dense.R +++ b/tests/testthat/test-fetch-dense.R @@ -19,7 +19,7 @@ for (path in test_files) { expect_equal(sum_, 178263235) }) - test_that("HiCFile: fetch (dense) cis", { + test_that("HiCFile: fetch (dense) symmetric cis", { f <- File(path, 100000) m <- fetch(f, "chr2R:10,000,000-15,000,000", type="dense") @@ -31,6 +31,35 @@ for (path in test_files) { expect_equal(sum_, 6029333) }) + test_that("HiCFile: fetch (dense) asymmetric cis", { + f <- File(path, 100000) + + m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:5,000,000-20,000,000", type="dense") + + shape <- dim(m) + sum_ <- sum(m) + + expect_equal(shape, c(100, 150)) + expect_equal(sum_, 6287451) + + + m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:10,000,000-20,000,000", type="dense") + + shape <- dim(m) + sum_ <- sum(m) + + expect_equal(shape, c(100, 100)) + expect_equal(sum_, 761223) + + m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:0-15,000,000", type="dense") + + shape <- dim(m) + sum_ <- sum(m) + + expect_equal(shape, c(100, 150)) + expect_equal(sum_, 12607205) + }) + test_that("HiCFile: fetch (dense) cis BED queries", { f <- File(path, 100000) From 37b6e88d2e7b0b642ab3cfd9c6a81e2435d56ce2 Mon Sep 17 00:00:00 2001 From: Roberto Rossini <71787608+robomics@users.noreply.github.com> Date: Tue, 16 Apr 2024 13:29:27 +0200 Subject: [PATCH 3/4] Bugfix --- conanfile.txt | 1 + tests/testthat/test-fetch-dense.R | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/conanfile.txt b/conanfile.txt index 7436f87..51907ba 100644 --- a/conanfile.txt +++ b/conanfile.txt @@ -55,6 +55,7 @@ hdf5*:enable_cxx=False hdf5*:hl=False hdf5*:threadsafe=False hdf5*:parallel=False +hictk*:with_eigen=False highfive*:with_boost=False highfive*:with_eigen=False highfive*:with_opencv=False diff --git a/tests/testthat/test-fetch-dense.R b/tests/testthat/test-fetch-dense.R index b2df602..9e90cda 100644 --- a/tests/testthat/test-fetch-dense.R +++ b/tests/testthat/test-fetch-dense.R @@ -34,7 +34,7 @@ for (path in test_files) { test_that("HiCFile: fetch (dense) asymmetric cis", { f <- File(path, 100000) - m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:5,000,000-20,000,000", type="dense") + m <- fetch(f, "chr2L:0-10,000,000", "chr2L:5,000,000-20,000,000", type="dense") shape <- dim(m) sum_ <- sum(m) @@ -43,7 +43,7 @@ for (path in test_files) { expect_equal(sum_, 6287451) - m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:10,000,000-20,000,000", type="dense") + m <- fetch(f, "chr2L:0-10,000,000", "chr2L:10,000,000-20,000,000", type="dense") shape <- dim(m) sum_ <- sum(m) @@ -51,7 +51,7 @@ for (path in test_files) { expect_equal(shape, c(100, 100)) expect_equal(sum_, 761223) - m <- fetch(f, ("chr2L:0-10,000,000", "chr2L:0-15,000,000", type="dense") + m <- fetch(f, "chr2L:0-10,000,000", "chr2L:0-15,000,000", type="dense") shape <- dim(m) sum_ <- sum(m) From f9e13cfdcd387cbc543bcf5ff557e169f24e9822 Mon Sep 17 00:00:00 2001 From: Roberto Rossini <71787608+robomics@users.noreply.github.com> Date: Tue, 16 Apr 2024 13:29:39 +0200 Subject: [PATCH 4/4] Do not rely on uint overflows --- src/hictkr_file.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/hictkr_file.cpp b/src/hictkr_file.cpp index b62b6fc..0fbf136 100644 --- a/src/hictkr_file.cpp +++ b/src/hictkr_file.cpp @@ -255,10 +255,10 @@ static Rcpp::NumericMatrix fetch_as_matrix(const Selector &sel) { matrix.at(i1, i2) = tp.count; if (mirror_matrix) { - // Mirror matrix below diagonal - if (i2 - i1 < num_rows && i1 < num_cols && i2 < num_rows) { + const auto delta = i2 - i1; + if (delta >= 0 && delta < num_rows && i1 < num_cols && i2 < num_rows) { matrix.at(i2, i1) = tp.count; - } else if (i2 - i1 > num_cols && i1 < num_cols && i2 < num_rows) { + } else if ((delta < 0 || delta > num_cols) && i1 < num_cols && i2 < num_rows) { const auto i3 = static_cast(tp.bin2_id - row_offset); const auto i4 = static_cast(tp.bin1_id - col_offset);