diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f40150a8..7e09ca72 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -90,10 +90,25 @@ jobs: # second `-m` would do. That failure is invisible and lands in the worst # place: the release commit gets benched, seeding a duplicate CodSpeed # baseline. Prefix + identity fails safe where equality fails silent. + # + # The repository clause keeps this workflow out of forks. It triggers on + # push to `main`, and a contributor whose PR branch IS their fork's `main` + # gets the whole thing run inside their own repository on every push to that + # branch. That happened on #63: the release job bumped all nine packages and + # pushed a `chore(release): publish` commit onto the open pull request, + # authenticated with the fork's own GITHUB_TOKEN. Publishing to npm failed + # there for want of credentials, so nothing reached the registry, but the + # version commit still landed on the PR. A fork gets nothing useful from + # this workflow in any case. + # + # Guarding this job alone would gate the chain, since release needs build + # and publish needs release, but the release job repeats the condition: it + # is the one that writes to the repository. if: >- - github.event_name != 'push' + github.repository == 'cornerstonejs/codecs' + && (github.event_name != 'push' || !(startsWith(github.event.head_commit.message, 'chore(release): publish') - && github.event.head_commit.author.email == '41898282+github-actions[bot]@users.noreply.github.com') + && github.event.head_commit.author.email == '41898282+github-actions[bot]@users.noreply.github.com')) # Same wasm build as pr-checks.yml, minus the change detection: a release # publishes whatever moved, and dicom-codec ships ranges over every sibling, # so all dists must be current. @@ -184,6 +199,10 @@ jobs: # Versions, tests and pushes. NO id-token: write — `pnpm install` runs in # this job, and nothing it pulls in has any business holding a credential # that can publish to npm. Publishing happens in the next job. + # + # Redundant with the guard on build, which this job depends on, but this is + # the job that commits and pushes — see the note there. + if: github.repository == 'cornerstonejs/codecs' needs: build runs-on: ubuntu-latest permissions: diff --git a/packages/openjpeg/src/BufferStream.hpp b/packages/openjpeg/src/BufferStream.hpp index 59116e4f..0d990a29 100644 --- a/packages/openjpeg/src/BufferStream.hpp +++ b/packages/openjpeg/src/BufferStream.hpp @@ -46,21 +46,39 @@ opj_write_to_buffer (void* p_buffer, OPJ_SIZE_T p_nb_bytes, return n; } -static OPJ_SIZE_T -opj_skip_from_buffer (OPJ_SIZE_T len, opj_buffer_info_t* psrc) +/* + * Must match opj_stream_skip_fn exactly: OPJ_OFF_T (*)(OPJ_OFF_T, void*). + * OPJ_OFF_T is int64_t, so declaring this with OPJ_SIZE_T -- 32-bit under + * wasm32 -- made the cast below an (i64,i32)->i64 indirect call landing on an + * (i32,i32)->i32 table entry, which traps as "function signature mismatch". + * + * It only fires when a skip is actually delegated to this callback: + * opj_stream_read_skip serves anything within m_bytes_in_buffer straight from + * the 1MB chunk it has already read, so small JP2 box skips are invisible and + * only a skip past the buffered remainder -- a large tile-part, a large box -- + * reaches us. Hence a multi-tile image failing where the same image re-tiled + * to a single tile decodes fine. Native builds tolerated the mismatched call, + * so this only ever showed up in wasm. + */ +static OPJ_OFF_T +opj_skip_from_buffer (OPJ_OFF_T len, opj_buffer_info_t* psrc) { OPJ_SIZE_T n = psrc->buf + psrc->len - psrc->cur; + if (len < 0) + return (OPJ_OFF_T)-1; + if (n) { - if (n > len) - n = len; + if (n > (OPJ_SIZE_T)len) + n = (OPJ_SIZE_T)len; psrc->cur += n; + + return (OPJ_OFF_T)n; } - else - n = (OPJ_SIZE_T)-1; - return n; + /* buffer exhausted: cio.c compares the result against (OPJ_OFF_T)-1 */ + return (OPJ_OFF_T)-1; } static OPJ_BOOL diff --git a/packages/openjpeg/src/J2KDecoder.hpp b/packages/openjpeg/src/J2KDecoder.hpp index de9fcb58..30d51be8 100644 --- a/packages/openjpeg/src/J2KDecoder.hpp +++ b/packages/openjpeg/src/J2KDecoder.hpp @@ -847,6 +847,11 @@ class J2KDecoder { return; } + /* Finalize decompression before the guards destroy the codec/stream */ + if (!opj_end_decompress(l_codec, l_stream)) { + printf("[WARNING] opj_decompress: opj_end_decompress failed\n"); + } + if (image->color_space != OPJ_CLRSPC_SYCC && image->numcomps == 3 && image->comps[0].dx == image->comps[0].dy && image->comps[1].dx != 1) { @@ -863,8 +868,12 @@ class J2KDecoder { color_esycc_to_rgb(image); } - frameInfo_.width = image->x1; - frameInfo_.height = image->y1; + // x1/y1 are absolute reference-grid coordinates, not pixel counts. For + // an image with a nonzero offset they overstate the decoded size, and + // the copy loop below indexes comps[].data with it, reading past the + // component buffer openjpeg allocated. + frameInfo_.width = image->x1 - image->x0; + frameInfo_.height = image->y1 - image->y0; frameInfo_.componentCount = image->numcomps; if (frameInfo_.componentCount != 1 && frameInfo_.componentCount != 3) { throw std::runtime_error("unsupported J2K component count"); @@ -905,7 +914,6 @@ class J2KDecoder { decoded_.resize(destinationSize); // Convert from int32 to native size - int comp_num; for (int y = 0; y < sizeAtDecompositionLevel.height; y++) { size_t lineStartPixel = y * sizeAtDecompositionLevel.width; diff --git a/packages/openjpeg/test/cpp/CMakeLists.txt b/packages/openjpeg/test/cpp/CMakeLists.txt index bf1cdf4d..a2ad94e6 100644 --- a/packages/openjpeg/test/cpp/CMakeLists.txt +++ b/packages/openjpeg/test/cpp/CMakeLists.txt @@ -11,5 +11,5 @@ target_compile_features(cpptest PUBLIC cxx_std_14) set(CMAKE_CXX_FLAGS_RELEASE "-O3") # add include path to openjpeg -include_directories("../extern/openjpeg/src/lib/openjp2" "../build/extern/openjpeg/src/lib/openjp2" - "../extern/openjpeg/src/bin/common" "../build/extern/openjpeg/src/bin/common") +include_directories("../../extern/openjpeg/src/lib/openjp2" "../../build/extern/openjpeg/src/lib/openjp2" + "../../extern/openjpeg/src/bin/common" "../../build/extern/openjpeg/src/bin/common") diff --git a/packages/openjpeg/test/helpers/jp2.mjs b/packages/openjpeg/test/helpers/jp2.mjs new file mode 100644 index 00000000..395745f9 --- /dev/null +++ b/packages/openjpeg/test/helpers/jp2.mjs @@ -0,0 +1,130 @@ +// Builds a JP2 (box-wrapped) file around a bare J2K codestream. +// +// Why this is generated rather than committed as a fixture: the point of the +// JP2 wrapper here is to make openjpeg delegate a skip to our own +// opj_skip_from_buffer callback in BufferStream.hpp, and that only happens for +// a skip LARGER than the stream's internal buffer. opj_stream_read_data always +// refills a full OPJ_J2K_STREAM_CHUNK_SIZE (1MB) chunk, and +// opj_stream_read_skip serves anything within m_bytes_in_buffer directly, so +// the skipped box has to be over 1MB. That makes the file too big to want in +// git, and it is trivially reproducible from a codestream we already ship. +// +// Run directly to write one out, e.g. to hand to a bug report: +// node test/helpers/jp2.mjs test/fixtures/j2k/CT1.j2k /tmp/CT1-boxed.jp2 + +import { readFileSync, writeFileSync } from "node:fs" +import { pathToFileURL } from "node:url" + +// opj_stream_default_create's buffer size (openjpeg.h OPJ_J2K_STREAM_CHUNK_SIZE). +export const STREAM_CHUNK_SIZE = 0x100000 + +// Comfortably over one chunk, so the skip cannot be served from the buffer no +// matter how much of it the header reads happen to have consumed. +export const DEFAULT_FILLER_BYTES = STREAM_CHUNK_SIZE + 4096 + +function box(type, ...contents) { + const content = Buffer.concat(contents) + const header = Buffer.alloc(8) + header.writeUInt32BE(content.length + 8, 0) + header.write(type, 4, 4, "ascii") + return Buffer.concat([header, content]) +} + +function u32(value) { + const b = Buffer.alloc(4) + b.writeUInt32BE(value, 0) + return b +} + +function u16(value) { + const b = Buffer.alloc(2) + b.writeUInt16BE(value, 0) + return b +} + +/** + * Reads the SIZ marker segment of a raw J2K codestream, so the JP2 image + * header we synthesise describes the codestream it actually wraps. + */ +export function parseSiz(codestream) { + if (codestream.readUInt16BE(0) !== 0xff4f || codestream.readUInt16BE(2) !== 0xff51) { + throw new Error("not a raw J2K codestream (expected SOC then SIZ)") + } + + const xsiz = codestream.readUInt32BE(8) + const ysiz = codestream.readUInt32BE(12) + const xosiz = codestream.readUInt32BE(16) + const yosiz = codestream.readUInt32BE(20) + const numComponents = codestream.readUInt16BE(40) + + // Ssiz is (bitdepth - 1) with the top bit set when signed — the same + // encoding JP2's ihdr BPC field uses, so it can be copied across verbatim. + const ssiz = codestream.readUInt8(42) + + return { + width: xsiz - xosiz, + height: ysiz - yosiz, + numComponents, + ssiz, + bitsPerSample: (ssiz & 0x7f) + 1, + isSigned: (ssiz & 0x80) !== 0, + } +} + +/** + * Wraps a raw J2K codestream in the minimum set of JP2 boxes, with a filler + * box in front of the codestream that openjpeg has no handler for and will + * therefore skip (jp2.c, the unknown-box branch of opj_jp2_read_header). + * + * @param {Buffer} codestream a bare .j2k codestream + * @param {{fillerBytes?: number}} [options] + * @returns {Buffer} a JP2 file + */ +export function wrapInJp2(codestream, { fillerBytes = DEFAULT_FILLER_BYTES } = {}) { + const siz = parseSiz(codestream) + + const signature = box("jP ", Buffer.from([0x0d, 0x0a, 0x87, 0x0a])) + const fileType = box("ftyp", Buffer.from("jp2 ", "ascii"), u32(0), Buffer.from("jp2 ", "ascii")) + + const ihdr = box( + "ihdr", + u32(siz.height), + u32(siz.width), + u16(siz.numComponents), + Buffer.from([ + siz.ssiz, + 7, // C: compression type, always 7 + 0, // UnkC: colourspace is known + 0, // IPR: no intellectual property rights box + ]) + ) + // METH=1 (enumerated), PREC=0, APPROX=0, then EnumCS. + const enumCs = siz.numComponents >= 3 ? 16 /* sRGB */ : 17 /* greyscale */ + const colr = box("colr", Buffer.from([1, 0, 0]), u32(enumCs)) + + // A 'free' box is exactly this: padding with no defined meaning. openjpeg + // has no handler for it, so it takes the skip path we are trying to reach. + const filler = box("free", Buffer.alloc(fillerBytes)) + + return Buffer.concat([ + signature, + fileType, + box("jp2h", ihdr, colr), + filler, + box("jp2c", codestream), + ]) +} + +// CLI: node test/helpers/jp2.mjs [fillerBytes] +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + const [input, output, fillerBytes] = process.argv.slice(2) + if (!input || !output) { + console.error("usage: node test/helpers/jp2.mjs [fillerBytes]") + process.exit(1) + } + const jp2 = wrapInJp2(readFileSync(input), { + fillerBytes: fillerBytes ? Number(fillerBytes) : undefined, + }) + writeFileSync(output, jp2) + console.log(`wrote ${output} (${jp2.length} bytes)`) +} diff --git a/packages/openjpeg/test/jp2.test.js b/packages/openjpeg/test/jp2.test.js new file mode 100644 index 00000000..af313b2d --- /dev/null +++ b/packages/openjpeg/test/jp2.test.js @@ -0,0 +1,180 @@ +// Regression coverage for the buffer stream's skip callback. +// +// opj_skip_from_buffer in src/BufferStream.hpp is handed to openjpeg as an +// opj_stream_skip_fn, which is OPJ_OFF_T(OPJ_OFF_T, void*). It used to be +// declared with OPJ_SIZE_T, and OPJ_SIZE_T is 32-bit under wasm32 while +// OPJ_OFF_T is int64_t, so the cast produced an (i64,i32)->i64 indirect call +// onto an (i32,i32)->i32 table entry. wasm traps that as +// "function signature mismatch" — see issue #62 and +// cornerstoneWADOImageLoader#400. +// +// Reaching the callback needs a skip bigger than the stream's 1MB internal +// buffer, because opj_stream_read_skip serves anything smaller straight out of +// it. wrapInJp2 arranges exactly that with an oversized 'free' box ahead of the +// codestream; see test/helpers/jp2.mjs. None of the .j2k fixtures trigger it, +// which is why the bug survived so long. +// +// Verified against a deliberately unfixed build: the two wasm targets throw +// RuntimeError, and the asm.js target is worse — it does not trap at all, it +// just reports width/height 0 and returns pixels that do not match the same +// codestream decoded bare. So this must assert on the decoded output, not +// merely that decode() did not throw. +import { beforeAll, describe, expect, it } from "vitest" +import { existsSync, readFileSync } from "node:fs" +import { fileURLToPath } from "node:url" +import { dirname, resolve } from "node:path" + +import { STREAM_CHUNK_SIZE, wrapInJp2 } from "./helpers/jp2.mjs" + +const __dirname = dirname(fileURLToPath(import.meta.url)) +const distDir = resolve(__dirname, "../dist") +const fixturesDir = resolve(__dirname, "fixtures") + +const ct1Encoded = readFileSync(resolve(fixturesDir, "j2k/CT1.j2k")) +const ct1Raw = readFileSync(resolve(fixturesDir, "raw/CT1.RAW")) + +// Built once here rather than per-variant: it is ~1.2MB of Buffer work. +const ct1Jp2 = wrapInJp2(ct1Encoded) + +async function loadModule(modulePath) { + const mod = await import(modulePath) + const factory = mod.default ?? mod + return await factory() +} + +const buildVariants = [ + { name: "asm.js full (openjpegjs)", path: "../dist/openjpegjs.js", dist: "openjpegjs.js" }, + { name: "wasm full (openjpegwasm)", path: "../dist/openjpegwasm.js", dist: "openjpegwasm.js" }, + { name: "wasm decode-only", path: "../dist/openjpegwasm_decode.js", dist: "openjpegwasm_decode.js" }, +] + +/** Walks a JP2 box list, returning each box's type and its content slice. */ +function readBoxes(buffer) { + const boxes = [] + let offset = 0 + while (offset < buffer.length) { + const length = buffer.readUInt32BE(offset) + // A length of 0 or 1 means "to end of file" / "64-bit length"; the builder + // emits neither, and treating them as normal would loop forever. + if (length < 8) throw new Error(`degenerate box length ${length} at ${offset}`) + boxes.push({ + type: buffer.toString("ascii", offset + 4, offset + 8), + length, + content: buffer.subarray(offset + 8, offset + length), + }) + offset += length + } + if (offset !== buffer.length) { + throw new Error(`boxes overrun the buffer: ended at ${offset} of ${buffer.length}`) + } + return boxes +} + +describe("JP2 test fixture construction", () => { + const boxes = readBoxes(ct1Jp2) + + it("puts a skipped box larger than the stream buffer ahead of the codestream", () => { + // If this stops holding, the decode tests below still pass but stop + // covering the skip callback at all — openjpeg would serve the skip from + // its internal buffer and never call into BufferStream.hpp. readBoxes also + // asserts the lengths tile the file exactly. + expect(boxes.map((b) => b.type)).toEqual(["jP ", "ftyp", "jp2h", "free", "jp2c"]) + + const free = boxes.find((b) => b.type === "free") + expect(free.content.length).toBeGreaterThan(STREAM_CHUNK_SIZE) + }) + + it("describes the wrapped codestream in the image header", () => { + // A jp2h that disagrees with the codestream makes openjpeg fail for + // reasons that have nothing to do with skipping. + const jp2h = boxes.find((b) => b.type === "jp2h") + const ihdr = readBoxes(jp2h.content).find((b) => b.type === "ihdr") + + expect(ihdr.content.length).toBe(14) // opj_jp2_read_ihdr rejects any other size + expect(ihdr.content.readUInt32BE(0)).toBe(512) // HEIGHT + expect(ihdr.content.readUInt32BE(4)).toBe(512) // WIDTH + expect(ihdr.content.readUInt16BE(8)).toBe(1) // NC + expect(ihdr.content.readUInt8(10)).toBe(0x8f) // BPC: signed 16-bit + }) + + it("embeds the codestream unchanged", () => { + const jp2c = boxes.find((b) => b.type === "jp2c") + expect(jp2c.content.equals(ct1Encoded)).toBe(true) + }) +}) + +describe.each(buildVariants)("openjpeg JP2 decode — $name", ({ path, dist }) => { + const isBuilt = existsSync(resolve(distDir, dist)) + let codec + + beforeAll(async () => { + if (isBuilt) codec = await loadModule(path) + }) + + it.runIf(process.env.CI)("dist is present in CI", () => { + expect(isBuilt, `${dist} missing — build artifact was not replayed`).toBe(true) + }) + + it.skipIf(!isBuilt)( + "decodes a JP2 whose skipped box exceeds the stream buffer (issue #62)", + () => { + const decoder = new codec.J2KDecoder() + decoder.getEncodedBuffer(ct1Jp2.length).set(ct1Jp2) + + // Before the BufferStream.hpp fix: RuntimeError on the wasm targets, + // and a silent width/height of 0 on asm.js. + decoder.decode() + + const frameInfo = decoder.getFrameInfo() + expect(frameInfo.width).toBe(512) + expect(frameInfo.height).toBe(512) + expect(frameInfo.bitsPerSample).toBe(16) + expect(frameInfo.componentCount).toBe(1) + + const decoded = decoder.getDecodedBuffer() + expect(decoded.length).toBe(ct1Raw.length) + expect(Buffer.from(decoded).equals(ct1Raw)).toBe(true) + + decoder.delete() + } + ) + + it.skipIf(!isBuilt)("decodes the JP2 to the same pixels as the bare codestream", () => { + const fromJp2 = new codec.J2KDecoder() + fromJp2.getEncodedBuffer(ct1Jp2.length).set(ct1Jp2) + fromJp2.decode() + const jp2Pixels = Buffer.from(fromJp2.getDecodedBuffer()) + + const fromJ2k = new codec.J2KDecoder() + fromJ2k.getEncodedBuffer(ct1Encoded.length).set(ct1Encoded) + fromJ2k.decode() + const j2kPixels = Buffer.from(fromJ2k.getDecodedBuffer()) + + expect(jp2Pixels.equals(j2kPixels)).toBe(true) + + fromJp2.delete() + fromJ2k.delete() + }) + + // Truncated here means the 'free' box header survives but its content and + // the codestream do not. That is handled by cio.c's own end-of-stream guard + // (it refuses to skip past m_user_data_length) rather than by our callback, + // so this is a malformed-input robustness check, not skip coverage. + it.skipIf(!isBuilt)("does not crash when a box claims to run past the end", () => { + const truncated = wrapInJp2(ct1Encoded).subarray(0, 32 + 45 + 8 + 1024) + + const decoder = new codec.J2KDecoder() + decoder.getEncodedBuffer(truncated.length).set(truncated) + + expect(() => { + try { + decoder.decode() + } catch { + // failing to decode a truncated file is the expected outcome; not + // trapping or corrupting the heap is what is being asserted + } + }).not.toThrow() + + decoder.delete() + }) +})