Fix `JpxImage` API issues (PR 17946 follow-up) #17951

timvandermeij · 2024-04-16T14:36:30Z

This commit changes the JpxImage.decode method signature to define the ignoreColorSpace argument as optional with a default value. Note that we already set this default value in the getBytes method of the src/core/decode_stream.js file since this option only seems useful for certain special cases and therefore shouldn't be mandatory to provide.

Moreover, the JPX fuzzer is changed to use the new JpxImage API.

timvandermeij · 2024-04-16T14:39:13Z

This PR is in draft because updating the fuzzer seems to have uncovered another problem related to the OpenJPEG integration:

$ npx jazzer fuzz/jpx_image.fuzz --sync
Dictionary: 4 entries
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1056338914
INFO: Loaded 1 modules   (512 inline 8-bit counters): 512 [0x7b37064ff010, 0x7b37064ff210), 
INFO: Loaded 1 PC tables (512 PCs): 512 [0x7b36fda00010,0x7b36fda02010), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
==19424== Uncaught Exception: ReferenceError: self is not defined
    at OpenJPEG (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/build/image_decoders/webpack:/pdf.js/external/openjpeg/openjpeg.js:8:811)
    at Function.decode (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/build/image_decoders/webpack:/pdf.js/src/core/jpx.js:29:22)
    at fuzz (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/fuzz/jpx_image.fuzz.js:21:1

I have also managed to reproduce this outside of the fuzzer:

Run npx gulp image_decoders.
Create a file named repro.js with this code:

import { JpxImage } from "./build/image_decoders/pdf.image_decoders.mjs";

JpxImage.decode(new Uint8Array([0]));

Run node repro.js.

This gives the following output:

$ node repro.js 
file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/build/image_decoders/pdf.image_decoders.mjs:5378
        scriptDirectory = self.location.href;
        ^

ReferenceError: self is not defined
    at file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/build/image_decoders/pdf.image_decoders.mjs:5378:9
    at JpxImage.decode (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/build/image_decoders/pdf.image_decoders.mjs:5795:22)
    at file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/repro.js:3:10
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:120:12

@calixteman Do you have an idea what could cause this self problem in the built OpenJPEG file?

calixteman · 2024-04-16T14:43:54Z

I wonder if it could be related to:
https://github.com/mozilla/pdf.js.openjpeg/blob/main/compile.sh#L24

Maybe we should change it for web.

timvandermeij · 2024-04-16T14:45:23Z

The self bit in the built OpenJPEG file does seem to be related to that variable, yes. I have edited the comment above to provide a minimal reproducer outside of the fuzzer that can easily reproduce the issue with a plain image decoders build and Node; does that help to confirm your thought?

The code surrounding self there is this:

    if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
      if (ENVIRONMENT_IS_WORKER) {
        scriptDirectory = self.location.href;
      } else if (typeof document != "undefined" && document.currentScript) {
        scriptDirectory = document.currentScript.src;
      }

Snuffleupagus · 2024-04-16T14:45:49Z

Maybe we should change it for web.

Please don't forget about our Node.js support as well.

calixteman · 2024-04-16T14:53:00Z

I confirm it's because of the workerenvironment.
Setting it to web fixes the issue and it's ok with either local dev server or m-c build.
I'm making a patch.

calixteman · 2024-04-16T14:58:04Z

I tested my change with the repro.js made by Tim in Node and I get:

OpenJPEG: Unknown format

node:internal/process/esm_loader:40
      internalBinding('errors').triggerUncaughtException(
                                ^
Error
    at BaseExceptionClosure (file:///home/calixte/dev/mozilla/pdf.js.calixteman/build/image_decoders/pdf.image_decoders.mjs:415:29)
    at file:///home/calixte/dev/mozilla/pdf.js.calixteman/build/image_decoders/pdf.image_decoders.mjs:418:2
    at ModuleJob.run (node:internal/modules/esm/module_job:195:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:336:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:106:12) {
  message: 'JPX error: JPX decode failed',
  name: 'JpxError'
}

Node.js v18.19.1

So I guess it works in Node since the string OpenJPEG: Unknown format is dumped from the C code.

timvandermeij · 2024-04-16T15:02:06Z

Yes, that seems good. The fuzzer has code to ignore such errors (because the data in my reproducer is clearly invalid) but I have stripped all that to provide a minimal script. Nice find!

This commit changes the `JpxImage.decode` method signature to define the `ignoreColorSpace` argument as optional with a default value. Note that we already set this default value in the `getBytes` method of the `src/core/decode_stream.js` file since this option only seems useful for certain special cases and therefore shouldn't be mandatory to provide. Moreover, the JPX fuzzer is changed to use the new `JpxImage` API.

timvandermeij · 2024-04-16T16:09:05Z

I have rebased the patch on top of #17954 and confirmed that the fuzzer now works. I have created #17955 to improve the fuzzer output.

If this PR is merged all JPX code should be functional, and we can then look into #17955 and any reported API issues (if they arise) in a separate scope.

timvandermeij · 2024-04-16T16:10:28Z

/botio test

moz-tools-bot · 2024-04-16T16:10:30Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/1f5eda7f52f4ef6/output.txt

moz-tools-bot · 2024-04-16T16:10:30Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/32892d2cf177f38/output.txt

moz-tools-bot · 2024-04-16T16:37:21Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/1f5eda7f52f4ef6/output.txt

Total script time: 26.84 mins

Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 23
  different first/second rendering: 3

Image differences available at: http://54.241.84.105:8877/1f5eda7f52f4ef6/reftest-analyzer.html#web=eq.log

moz-tools-bot · 2024-04-16T16:49:28Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/32892d2cf177f38/output.txt

Total script time: 38.95 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 5
  different first/second rendering: 1

Image differences available at: http://54.193.163.58:8877/32892d2cf177f38/reftest-analyzer.html#web=eq.log

timvandermeij added the image-jpx label Apr 16, 2024

timvandermeij requested review from Snuffleupagus and calixteman April 16, 2024 14:36

timvandermeij force-pushed the fix-jpx-decoder branch from 52e361d to cdd8c64 Compare April 16, 2024 14:47

timvandermeij force-pushed the fix-jpx-decoder branch from cdd8c64 to c08b09d Compare April 16, 2024 16:06

timvandermeij marked this pull request as ready for review April 16, 2024 16:09

Snuffleupagus approved these changes Apr 16, 2024

View reviewed changes

timvandermeij merged commit 921fae5 into mozilla:master Apr 16, 2024
9 checks passed

timvandermeij deleted the fix-jpx-decoder branch April 16, 2024 17:09

timvandermeij removed the request for review from calixteman April 16, 2024 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `JpxImage` API issues (PR 17946 follow-up) #17951

Fix `JpxImage` API issues (PR 17946 follow-up) #17951

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 •

edited

Loading

calixteman commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 •

edited

Loading

Snuffleupagus commented Apr 16, 2024

calixteman commented Apr 16, 2024

calixteman commented Apr 16, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 •

edited

Loading

timvandermeij commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

Fix JpxImage API issues (PR 17946 follow-up) #17951

Fix JpxImage API issues (PR 17946 follow-up) #17951

Conversation

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 • edited Loading

calixteman commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 • edited Loading

Snuffleupagus commented Apr 16, 2024

calixteman commented Apr 16, 2024

calixteman commented Apr 16, 2024

timvandermeij commented Apr 16, 2024

timvandermeij commented Apr 16, 2024 • edited Loading

timvandermeij commented Apr 16, 2024

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Apr 16, 2024

From: Bot.io (Windows)

Failed

Fix `JpxImage` API issues (PR 17946 follow-up) #17951

Fix `JpxImage` API issues (PR 17946 follow-up) #17951

timvandermeij commented Apr 16, 2024 •

edited

Loading

timvandermeij commented Apr 16, 2024 •

edited

Loading

timvandermeij commented Apr 16, 2024 •

edited

Loading