Skip to content
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.

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

openjpeg: update to 2.3.1 #5079

Merged
merged 1 commit into from
Sep 10, 2019
Merged

Conversation

timmooney
Copy link
Contributor

This is currently a work in progress -- I need some direction regarding what to do about one of the outdated patches. The latest version includes security fixes, and is needed for other packages I would like to rebuild and update.

The patch patches/openjpeg-02-mct-sse.patch does not apply. It looks like the section of code it relates to is still present. However, it looks like the patch we're using may have come from "PDFium", and it appears they later removed that patch: PDFium bug

I can't build with the existing patch, so it either needs to be removed or it needs to be updated.

@Mno-hime
Copy link
Contributor

The patch patches/openjpeg-02-mct-sse.patch does not apply. It looks like the section of code it relates to is still present. However, it looks like the patch we're using may have come from "PDFium", and it appears they later removed that patch: PDFium bug

I can't build with the existing patch, so it either needs to be removed or it needs to be updated.

Perhaps we don't need any of them? At least Solaris does not and we may be very close to that: https://github.com/oracle/solaris-userland/tree/master/components/desktop/openjpeg.

From other suggestions:

  1. Perhaps there are some goodies in the solaris-userland Makefile we might want to bring over.
  2. Upstream seems to have some tests, it's worth to investigate if we can leverage them cheaply.

Are you going to rebuild it's dependencies? I found library/audio/gstreamer1/plugin/bad, library/libpoppler, and video/ffmpeg. Although openjpeg changelog says it's API/ABI compatible with v2.1, they added new APIs/ABIs, which may break things. (ABI Tracker does not follow openjpeg unfortunately.)

@timmooney
Copy link
Contributor Author

Thanks for the comments and review, Michal. It's appreciated.

I didn't know whether there were any copyright or licensing issues that would keep me from looking at Oracle's work, so up to this point, I've never looked in solaris-userland. I do sometimes look at what one or more Linux distributions include for patches or configure/packaging options, since their patches and build recipes are generally very liberally licensed.

If comparing solaris-userland to hipster is OK to do in all cases, I'll make a point of looking at solaris-userland when I run into more issues like these.

Our existing component doesn't run the test suite, but I did try the tests when I build openjpeg without the mct.c/SSE patch. The first 27 tests pass, and then next 1200+ tests fail. I didn't look closely at those tests, but they all seemed to be doing md5sums on resulting files. I expect there are very minor differences in the files between our test results and the expected, but of course they're enough to fail an md5sum, but that's just a guess. I would have to look more closely at the tests.

I did notice that SONAME has not changed since 2.1.0, but to be safe I had been planning on doing dependency rebuilds (once I had some direction on what to do about the SSE patch).

@alarcher
Copy link
Contributor

@Mno-hime @timmooney I created the patch partly based on PDFium but their patch was incorrect so I modified it. Later on a similar patch was picked by Fedora but they made a mistake, other distributions like Ubuntu did not have this patch despite bug reports. If the alignment is enforced now outside of this function call then it is not useful anymore but I would suggest to verify that.

@timmooney
Copy link
Contributor Author

Thanks much for that background information @alarcher , it's very useful to know.

Looking at openjpeg issue 625 and openjpeg issue 624 , my interpretation of the comments is that they've addressed this by ensuring the correct alignment -- it's not super clear, though.

@Mno-hime
Copy link
Contributor

I didn't know whether there were any copyright or licensing issues that would keep me from looking at Oracle's work, so up to this point, I've never looked in solaris-userland. I do sometimes look at what one or more Linux distributions include for patches or configure/packaging options, since their patches and build recipes are generally very liberally licensed.

If comparing solaris-userland to hipster is OK to do in all cases, I'll make a point of looking at solaris-userland when I run into more issues like these.

I believe it's OK to look at and take from solaris-userland. Their userland is under the same CDDL license as ours. Just make sure Oracle copyright is brought over to oi-userland, if something is copied.

The other good source of patches & knowledge is pkgsrc (https://github.com/NetBSD/pkgsrc/).

Our existing component doesn't run the test suite, but I did try the tests when I build openjpeg without the mct.c/SSE patch. The first 27 tests pass, and then next 1200+ tests fail. I didn't look closely at those tests, but they all seemed to be doing md5sums on resulting files. I expect there are very minor differences in the files between our test results and the expected, but of course they're enough to fail an md5sum, but that's just a guess. I would have to look more closely at the tests.

It's a good idea to pursue this further. Once you are able to run the test suite via gmake test, push the changes here, and maybe I'll be of assistance.

I did notice that SONAME has not changed since 2.1.0, but to be safe I had been planning on doing dependency rebuilds (once I had some direction on what to do about the SSE patch).

I believe this is OK, unless the major SONAME number changed.

@timmooney
Copy link
Contributor Author

I've made progress with the test suite for openjpeg-2.3.1. Most of the tests in the suite require that you've also downloaded openjpeg-data , the conformance data. That bundle of test files is 600+ MiB in size.

With the conformance data in place, when building 32-bit, there are 9 test failures:

99% tests passed, 9 tests failed out of 1481

Total Test time (real) = 175.72 sec

The following tests FAILED:
        101 - NR-JP2-file2.jp2-compare2base (Failed)
        104 - NR-JP2-file3.jp2-compare2base (Failed)
        1018 - NR-DEC-file409752.jp2-40-decode-md5 (Failed)
        1024 - NR-DEC-issue205.jp2-43-decode-md5 (Failed)
        1056 - NR-DEC-issue208.jp2-69-decode-md5 (Failed)
        1064 - NR-DEC-issue226.j2k-74-decode (Failed)
        1065 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
        1381 - NR-DEC-issue236-ESYCC-CDEF.jp2-254-decode-md5 (Failed)
        1475 - NR-DEC-db11217111510058.jp2-306-decode-md5 (Failed)
Errors while running CTest

However, when building 64-bit there are only 2 test failures:

99% tests passed, 2 tests failed out of 1481

Total Test time (real) = 144.92 sec

The following tests FAILED:
        1064 - NR-DEC-issue226.j2k-74-decode (Failed)
        1065 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
Errors while running CTest

How would you like to proceed?

@timmooney
Copy link
Contributor Author

To give us a baseline for comparison, I went back to openjpeg-2.1.2, our current component version, and added the test suite to that. I then tried both 32 bit and 64 bit builds, both with and without the SSE patch patches/openjpeg-02-mct-sse.patch:

Build 32-bit 64-bit
openjpeg-2.1.2 with SSE patch patches/openjpeg-02-mct-sse.patch 36 failures 32 failures
openjpeg-2.1.2 without SSE patch patches/openjpeg-02-mct-sse.patch 18 failures 12 failures

So at least for the test suite, our current shipping version would have fewer failures if we didn't apply the SSE patch.

The detailed results for each case are:

openjpeg-2.1.2, 32 bit, without the SSE patch in openjpeg-02-mct-sse.patch:

99% tests passed, 18 tests failed out of 1409

Total Test time (real) = 433.84 sec

The following tests FAILED:
     89 - NR-JP2-file2.jp2-compare2base (Failed)
     92 - NR-JP2-file3.jp2-compare2base (Failed)
    445 - NR-issue823.jp2-compare_dump2base (Failed)
    955 - NR-DEC-issue104_jpxstream.jp2-33-decode-md5 (Failed)
    964 - NR-DEC-file409752.jp2-40-decode-md5 (Failed)
    970 - NR-DEC-issue205.jp2-43-decode-md5 (Failed)
    1002 - NR-DEC-issue208.jp2-69-decode-md5 (Failed)
    1009 - NR-DEC-issue226.j2k-74-decode (Failed)
    1010 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
    1132 - NR-DEC-p1_06.j2k-156-decode (Failed)
    1133 - NR-DEC-p1_06.j2k-156-decode-md5 (Failed)
    1148 - NR-DEC-p1_06.j2k-164-decode (Failed)
    1149 - NR-DEC-p1_06.j2k-164-decode-md5 (Failed)
    1328 - NR-DEC-issue236-ESYCC-CDEF.jp2-254-decode-md5 (Failed)
    1403 - Found-But-No-Test-db11217111510058.jp2 (Failed)
    1404 - Found-But-No-Test-issue979.j2k (Failed)
    1405 - Found-But-No-Test-oss-fuzz2785.jp2 (Failed)
    1406 - Found-But-No-Test-tnsot_zero.jp2 (Failed)
Errors while running CTest

openjpeg-2.1.2, 32 bit with the SSE patch openjpeg-02-mct-sse.patch:

97% tests passed, 36 tests failed out of 1409

Total Test time (real) = 392.36 sec

The following tests FAILED:
         76 - ETS-C1P1-p1_05.j2k-compare2ref (Failed)
         77 - NR-C1P1-p1_05.j2k-compare2base (Failed)
         79 - ETS-C1P1-p1_06.j2k-compare2ref (Failed)
         80 - NR-C1P1-p1_06.j2k-compare2base (Failed)
         89 - NR-JP2-file2.jp2-compare2base (Failed)
         92 - NR-JP2-file3.jp2-compare2base (Failed)
        184 - ETS-RIC-zoo2.jp2-compare2ref (Failed)
        185 - NR-RIC-zoo2.jp2-compare2base (Failed)
        445 - NR-issue823.jp2-compare_dump2base (Failed)
        955 - NR-DEC-issue104_jpxstream.jp2-33-decode-md5 (Failed)
        964 - NR-DEC-file409752.jp2-40-decode-md5 (Failed)
        968 - NR-DEC-issue206_image-000.jp2-42-decode-md5 (Failed)
        970 - NR-DEC-issue205.jp2-43-decode-md5 (Failed)
        1002 - NR-DEC-issue208.jp2-69-decode-md5 (Failed)
        1009 - NR-DEC-issue226.j2k-74-decode (Failed)
        1010 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
        1105 - NR-DEC-p1_06.j2k-142-decode-md5 (Failed)
        1107 - NR-DEC-p1_06.j2k-143-decode-md5 (Failed)
        1109 - NR-DEC-p1_06.j2k-144-decode-md5 (Failed)
        1111 - NR-DEC-p1_06.j2k-145-decode-md5 (Failed)
        1113 - NR-DEC-p1_06.j2k-146-decode-md5 (Failed)
        1115 - NR-DEC-p1_06.j2k-147-decode-md5 (Failed)
        1132 - NR-DEC-p1_06.j2k-156-decode (Failed)
        1133 - NR-DEC-p1_06.j2k-156-decode-md5 (Failed)
        1135 - NR-DEC-p1_06.j2k-157-decode-md5 (Failed)
        1137 - NR-DEC-p1_06.j2k-158-decode-md5 (Failed)
        1139 - NR-DEC-p1_06.j2k-159-decode-md5 (Failed)
        1141 - NR-DEC-p1_06.j2k-160-decode-md5 (Failed)
        1148 - NR-DEC-p1_06.j2k-164-decode (Failed)
        1149 - NR-DEC-p1_06.j2k-164-decode-md5 (Failed)
        1326 - NR-DEC-issue205.jp2-253-decode-md5 (Failed)
        1328 - NR-DEC-issue236-ESYCC-CDEF.jp2-254-decode-md5 (Failed)
        1403 - Found-But-No-Test-db11217111510058.jp2 (Failed)
        1404 - Found-But-No-Test-issue979.j2k (Failed)
        1405 - Found-But-No-Test-oss-fuzz2785.jp2 (Failed)
        1406 - Found-But-No-Test-tnsot_zero.jp2 (Failed)
Errors while running CTest

openjpeg-2.1.2, 64-bit, without the SSE patch:

99% tests passed, 12 tests failed out of 1409

Total Test time (real) = 410.87 sec

The following tests FAILED:
        445 - NR-issue823.jp2-compare_dump2base (Failed)
        955 - NR-DEC-issue104_jpxstream.jp2-33-decode-md5 (Failed)
        1009 - NR-DEC-issue226.j2k-74-decode (Failed)
        1010 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
        1132 - NR-DEC-p1_06.j2k-156-decode (Failed)
        1133 - NR-DEC-p1_06.j2k-156-decode-md5 (Failed)
        1148 - NR-DEC-p1_06.j2k-164-decode (Failed)
        1149 - NR-DEC-p1_06.j2k-164-decode-md5 (Failed)
        1403 - Found-But-No-Test-db11217111510058.jp2 (Failed)
        1404 - Found-But-No-Test-issue979.j2k (Failed)
        1405 - Found-But-No-Test-oss-fuzz2785.jp2 (Failed)
        1406 - Found-But-No-Test-tnsot_zero.jp2 (Failed)
Errors while running CTest

openjpeg-2.1.2, 64-bit, with the SSE patch:

98% tests passed, 32 tests failed out of 1409

Total Test time (real) = 458.14 sec

The following tests FAILED:
     76 - ETS-C1P1-p1_05.j2k-compare2ref (Failed)
     77 - NR-C1P1-p1_05.j2k-compare2base (Failed)
     79 - ETS-C1P1-p1_06.j2k-compare2ref (Failed)
     80 - NR-C1P1-p1_06.j2k-compare2base (Failed)
    184 - ETS-RIC-zoo2.jp2-compare2ref (Failed)
    185 - NR-RIC-zoo2.jp2-compare2base (Failed)
    445 - NR-issue823.jp2-compare_dump2base (Failed)
    955 - NR-DEC-issue104_jpxstream.jp2-33-decode-md5 (Failed)
    968 - NR-DEC-issue206_image-000.jp2-42-decode-md5 (Failed)
    970 - NR-DEC-issue205.jp2-43-decode-md5 (Failed)
    1002 - NR-DEC-issue208.jp2-69-decode-md5 (Failed)
    1009 - NR-DEC-issue226.j2k-74-decode (Failed)
    1010 - NR-DEC-issue226.j2k-74-decode-md5 (Failed)
    1105 - NR-DEC-p1_06.j2k-142-decode-md5 (Failed)
    1107 - NR-DEC-p1_06.j2k-143-decode-md5 (Failed)
    1109 - NR-DEC-p1_06.j2k-144-decode-md5 (Failed)
    1111 - NR-DEC-p1_06.j2k-145-decode-md5 (Failed)
    1113 - NR-DEC-p1_06.j2k-146-decode-md5 (Failed)
    1115 - NR-DEC-p1_06.j2k-147-decode-md5 (Failed)
    1132 - NR-DEC-p1_06.j2k-156-decode (Failed)
    1133 - NR-DEC-p1_06.j2k-156-decode-md5 (Failed)
    1135 - NR-DEC-p1_06.j2k-157-decode-md5 (Failed)
    1137 - NR-DEC-p1_06.j2k-158-decode-md5 (Failed)
    1139 - NR-DEC-p1_06.j2k-159-decode-md5 (Failed)
    1141 - NR-DEC-p1_06.j2k-160-decode-md5 (Failed)
    1148 - NR-DEC-p1_06.j2k-164-decode (Failed)
    1149 - NR-DEC-p1_06.j2k-164-decode-md5 (Failed)
    1326 - NR-DEC-issue205.jp2-253-decode-md5 (Failed)
    1403 - Found-But-No-Test-db11217111510058.jp2 (Failed)
    1404 - Found-But-No-Test-issue979.j2k (Failed)
    1405 - Found-But-No-Test-oss-fuzz2785.jp2 (Failed)
    1406 - Found-But-No-Test-tnsot_zero.jp2 (Failed)
Errors while running CTest

@alarcher
Copy link
Contributor

alarcher commented Jul 3, 2019

@timmooney This is totally normal that there are more test failures with the patch, it does not mean that the code is broken. Bottom line is: if the memory is guaranteed to be aligned now then simply remove the patch, otherwise not as it will break reading some pictures like before. Thanks for your work :)

@Mno-hime
Copy link
Contributor

@timmooney Did you decided one way or the other? You may consider asking on some upstream mailing list for guidance.

@timmooney
Copy link
Contributor Author

Good suggestion @Mno-hime , I've posted to the OpenJPEG Google group asking for clarification.

I should know how to proceed once there's been some kind of response there.

@alarcher
Copy link
Contributor

@timmooney We can check on one of the offending PDFs submitted to the ML if the patch is still necessary. If not, just push it as-is.

@timmooney
Copy link
Contributor Author

Thanks, @alarcher . There's been no response to my question on the openjpeg googlegroup.

I don't know where to get the offending PDFs you're talking about. If you can provide a bit more direction I would be willing to do more testing.

@alarcher
Copy link
Contributor

Could you check the links at #1648 ?

@timmooney
Copy link
Contributor Author

Thanks for the pointer, I found the multi-part 7zip of a PDF that triggered the problem in illumos issue #6463 . I'll piece that together and test with it.

@timmooney
Copy link
Contributor Author

After testing, I think it's safe to push my changes without the patch to mct.c. Should I move patches/openjpeg-02-mct-sse.patch to some other spot (maybe old-patches?) and keep it around for historical purposes, or just trust that if someone wants to look at the old patch they'll be able to resurrect it with git?

I tested with the DrayTek_UG-Vigor2860_V3.1.pdf from illumos issue 6463.

The baseline for my tests:

  • open the PDF with atril and scroll through the entire document. atril doesn't use openjpeg directly, but it uses libpoppler which uses openjpeg.
  • mkdir /var/tmp/pdf-image-test-before; cd /var/tmp/pdf-image-test-before and then pdfimages -all DrayTek_UG-Vigor2860_V3.1.pdf images to extract every image to a separate file.

After that baseline, I installed openjpeg@2.3.1, did not rebuild poppler or any other packages, and ran the same tests again. Since the SONAME didn't change, I wanted to see if there were any obvious problems.

atril handled the document without crashing, and using pdfimages again to extract all the images worked fine. Doing a cmp between the "before" and "after" directories didn't turn up any differences.

I'm not currently planning on triggering rebuilds of any dependencies, since the SONAME didn't change.

@alarcher
Copy link
Contributor

After testing, I think it's safe to push my changes without the patch to mct.c. Should I move patches/openjpeg-02-mct-sse.patch to some other spot (maybe old-patches?) and keep it around for historical purposes, or just trust that if someone wants to look at the old patch they'll be able to resurrect it with git?

Thanks for the testing, we are good to go then :) the patch will stay in the git history, it should be fine.
Alternatively you could attach the patch to my original ticket in the bug tracker for safekeeping.

I'm not currently planning on triggering rebuilds of any dependencies, since the SONAME didn't change.

https://abi-laboratory.pro/index.php?view=timeline&l=openjpeg seems to agree with you :)
Given the testing performed I think I can merge whenever you are ready.
@Mno-hime are you OK with that?

@timmooney
Copy link
Contributor Author

timmooney commented Sep 3, 2019

My updated changes were auto-merged because of Michal's previous approval, which was a little unnerving. :-)

For reference, this fixes all of the OpenJPEG CVEs that mention "before 2.2.0" or "in 2.1.2" in their description.

@Mno-hime
Copy link
Contributor

Mno-hime commented Sep 4, 2019

@timmooney I am afraid we don't understand what you are saying. What you mean by "those changes being auto-merged"? As far as I know this wasn't merged. I am sure we are still interested in our contribution.

@timmooney
Copy link
Contributor Author

@Mno-hime I mean that the moment I force-pushed my updated changes, github appears to have both automatically merged the changes and automatically closed this issue. I didn't do anything to close this; it closed automatically as soon as my changes were pushed.

Also, the message I see says "Pull request sucessfully merged and closed".

This is the first time I've ever seen that happen, and I had assumed it was because you had previously approved an older change-set. It's of course possible that something else is going on and I'm misunderstanding what github has done.

@alarcher
Copy link
Contributor

alarcher commented Sep 4, 2019

@timmooney There are no commits on this branch, are you sure you pushed the updated changes to the right branch?

@timmooney
Copy link
Contributor Author

You're correct, I was not on the right branch. Sorry for the trouble.

I have fixed the issue in my local git repo and pushed the changes from the local repo to timmooney:openjpeg-update on github. When I look at my repositories, I can see that the openjpeg-update branch changed just a few minutes ago with the changes I pushed.

However, I expected the changes to automatically appear in this pull request, and they are not.

Any thoughts on what I should do to get the changes from my branch to appear in this pull?

@alarcher alarcher reopened this Sep 4, 2019
@Mno-hime
Copy link
Contributor

Mno-hime commented Sep 6, 2019

@timmooney I see 55b3ada in "Commits". Do you miss any other commits? Whenever I see something wrong with my PR, I rebase against upstream/oi/hipster and force-push.

@timmooney
Copy link
Contributor Author

@Mno-hime That is the single commit that's needed.

It's actually the rebase that gives me the most trouble. There are thousands of articles on git rebase, but there seems to be many different methods or approaches. I have yet to find one that I'm fully comfortable with. Since I only have time to contribute on an infrequent and unpredictable basis, I'm not getting as comfortable with git as quickly as I would if I could contribute every day.

When I can create my branch, make my changes, push to github, and then open a PR all before a bunch of stuff changes on master, then I usually don't have any problems. The problems start when a lot of time goes by after creating my branch, so lots of stuff has changed on master that isn't reflected in my branch.

Thanks for all the patient help from you and @alarcher .

@Mno-hime
Copy link
Contributor

Mno-hime commented Sep 6, 2019

This is what I do: git fetch upstream && git rebase -v upstream/oi/hipster. upstream being remote pointing to OpenIndiana/oi-userland via HTTP.

@Mno-hime
Copy link
Contributor

Mno-hime commented Sep 6, 2019

Anyway, I think we are good here. If you agree @timmooney, remove the "WiP" tag in the PR.

@timmooney timmooney changed the title openjpeg: [WiP] update to 2.3.1 openjpeg: update to 2.3.1 Sep 6, 2019
@alarcher alarcher merged commit 28c8532 into OpenIndiana:oi/hipster Sep 10, 2019
@timmooney
Copy link
Contributor Author

Thanks Aurélien! Sorry for the extra work you had to do on this one. I'll try avoid causing merge issues like this in the future!

@alarcher
Copy link
Contributor

This broke the ffmpeg build. Just a note to remind us to check it next time.

@timmooney timmooney deleted the openjpeg-update branch November 8, 2021 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants