-
Notifications
You must be signed in to change notification settings - Fork 35
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
Improve build: add CI, run xalan-test in CI, download jars from Central #2
Conversation
111f0f5
to
1c8dd1b
Compare
0c79f4d
to
54da5bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we should'nt be doing some of the changes, as suggested within the file build.xml. These have never, been problems with Xalan-J build.
Hello, I'm the maintainer of EchoXSL, a recent fork of Apache Xalan-J. I'm happy that the Xalan Project wants to produce an additional release with a fix for CVE-2022-34169, however the way it is done in this PR would be problematic for modular JDKs. As explained in css4j#2, the vulnerability belongs in fact to Apache BCEL, and Xalan is vulnerable because it bundles an old version of it in the jar (together with java-cup etc). However other software do also depend on BCEL or java-cup, and in modular projects this would lead to a split packages problem. Moreover if you just download the vulnerable BCEL jar file and add its packages, Xalan would still be vulnerable. My suggestion would be to fix the vulnerability in the BCEL project, and then set it up as a dependency in Xalan. This is the approach that EchoXSL followed and works fine. The resulting Maven POM should include something like this in the <dependency>
<groupId>org.apache.bcel</groupId>
<artifactId>bcel</artifactId>
<version>6.5.1</version>
<scope>compile</scope>
</dependency> Feel free to reuse the Gradle build in EchoXSL if it is of any help. |
@carlosame , I did not intend to fix CVEs in this PR. I just wanted to add CI so all the further modifications could be tested. |
It was tempting to rip off build.xml and replace it with Gradle, yet it sounds like a "too much" change for fixing a CVE :) |
@mukulga , would you please clarify which changes you suggest skipping? |
why would, changes specified as "Remove bootclasspath customization" within the PR for build.xml needed? May be, another XalanJ committer can suggest as well, on this point. |
What is the need for bootclasspath cusomization? As far as I remember, bootclasspath removal was needed when I tried building Xalan with Java 11. |
To "fix" the CVE, all you need to do is to remove the BCEL packages from the jar, and then list BCEL as a dependency. Now the CVE belongs to somebody else. And to be friendly to modular JDKs, you have to do the same for the rest of the foreign packages that are currently shipped with Xalan. All of this could be done in this PR. |
Replacing the embedded dependencies with external ones is not backward compatible change, so I would refrain from that for the next release. |
I reverted bootclasspath-related changes for now since it is not needed for Java 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that, the pr change, mentioned as, "Remove -static option from JLex call" shouldn't be done. This has not been, issue with XalanJ builds earlier.
It seems that, you're suggesting to enhance jlex version to 1.2.6 (which is latest jlex version as of 2003). Why we should do this jlex version upgrade on XalanJ, when the jlex jar stored on XalanJ repos is serving well for the current XalanJ codebase? I'd suggest that, lets not enhance versions of XalanJ supporting libraries, if XalanJ doesn't have known issues with current library versions that it uses.
Where did you get JLex.jar then? |
I could see, jlex jar available at https://github.com/apache/xalan-java/tree/xalan-j_2_7_1_maint/tools. I guess, we should be using this version for the next XalanJ release, unless there's a known jlex issue with current XalanJ codebase. |
@mukulga , the ASF policy forbids including binary (compiled, non-text) code in the source artifacts. https://www.apache.org/legal/release-policy.html#source-packages
|
@mukulga , please check https://lists.apache.org/thread/otx07h6vbjrsqd9r9sqpcpjscvjwtmfc
|
In this case I can replace "retrieval the offical JLex" with "download from https://github.com/apache/xalan-java/blob/xalan-j_2_7_1_maint/tools/JLex.jar", however, that is really moot license-wise. I think the proper resolution is to embed JLex sources into xalan-java source code, however, I'm inclined it should better be in another PR. |
An alternative option is to comment out the call to "jlex", and postpone the decision till xpath.lex modification would be needed. |
I commented the call to |
By the way, I would say XPathLexer violates that binary rule as well. xalan-java/src/org/apache/xalan/xsltc/compiler/XPathLexer.java Lines 566 to 598 in 222095d
|
I've created https://issues.apache.org/jira/browse/XALANJ-2635 regarding |
This seems to be mixing a number of issues -- the JLex question, the more general question of where the dependency jarfiles should live and how they are fetched, and running continuous integration. I'd be happier if we could divide these and address them separately. XPathLexer appears to be generated from xpath.lex using JLex, according to the build.xml file. See the property ${generated.xpathlexer" and its usage. So we DO have checked-in source for that in the Xalan-Java project. The problem is that we don't have either source, or a source, for JLex. Last I checked, it wasn't clear who actually owns/maintains JLex. If anyone. The proper solution would be to find a supported (or at least clearly open-sourced) Java lex implementation compatible with any JLex quirks (and/or to rework the input to work with the new lex) and swap it in. That's a somewhat scary proposal, deserving its own work item. Note that JLex and XPathLexer are part of the xsltc "compiled xslt processor" code, originally contributed to Apache by Sun Microsystems. I did a lot of the work to reconcile that code with Xalan and glue them together as a single system... but I didn't go very deeply into it at the time, just enough to sew the monster together. A significant portion of Sun's code was accepted unexamined before I even started that process, which is how JLex got brought in. Yes, these days Apache would insist on knowing the source of all the pieces, but things were a bit looser then; as long as Sun took responsibility for the code donation, we trusted that they had either written it themselves or sourced it ethically. Good luck finding someone at Sun who remembers where they got JLex from, especially since they pretty much vanished from the Xalan project after the integration. I think we just have to accept this as grandfathered code until/unless someone is willing to tackle either tracking it down (and dealing with any changes since we got our copy that might affect our use of it) or replacing it. Either way, I'd want to see that tested to death before committing to it. |
Thank you for the review, however, I truly do not understand if you want an action from me or not. I believe the commits are self-contained, so feel free to commit all of them or some of them to the main branch.
Please check https://issues.apache.org/jira/browse/XALANJ-2635 description. It includes the link to the official maintainer: https://www.cs.princeton.edu/~appel/modern/java/JLex/ I believe JLex is not connected with Sun Microsystems, so "Good luck finding someone at Sun who remembers where they got JLex from" comment does not apply. |
Re XALANJ-2635: Assuming that is the same JLex we're using, which I haven't verified, the "official maintainer" hasn't touched that page in two decades. I can try pinging them.... Copying the source for this tool locally is theoretically permitted by its license, but since it isn't one of the "standard" opensource licenses we might need an official OK. I see java_cup.jar is on Maven. If JLex got posted to Maven, or github, would that satisfy your request for provenance? (That would also make it more visible/available for non-Xalan users, of course.) Lemme take a longer look at this. |
OK, I'm a bit confused. For java_cup.jar, you aren't downloading it from Maven (even though it's listed as a Maven project); you're explicitly downloading from java_cup's page at Technische Universität München. I'm not following why the same basic solution -- but download and build rather than download and untar -- wouldn't be the right answer for JLex, fetching https://www.cs.princeton.edu/~appel/modern/java/JLex/Archive/1.2.6/Main.java or whichever other specific release is desired. Yes, there is risk that the JLex page Goes Away at some point. But it appears to be equivalent to the risk for java_cup. What am I missing? |
Please read #2 (comment)
xalan-java uses java-cup version 11b or something like that. |
Hm. Digressing, and apologies for playing catch-up here if the answer is "tried, can't", but... Have we tested with the newer java_cup release? Unless we have a specific reason for staying with a back level version, the usual Maven philosophy would be that we at least try to stay with current code, updating our own code to track it if necessary... "At least version X" is preferred to exact version unless there's a known issue.
The same would apply to JLex; if there's a way to avoid needing a copy which is both backdated and modified, we should consider that.
I presume we know what the modification was, it's not just that we have previous version of JLex (it doesn't announce version number), and it's too messy to apply programmatically do a downloaded copy?
…--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
() Plaintext Ribbon Campaign
/\ Stamp out HTML mail!
________________________________
From: Vladimir Sitnikov ***@***.***>
Sent: Wednesday, June 14, 2023 1:53:40 AM
To: apache/xalan-java ***@***.***>
Cc: Joe Kesselman ***@***.***>; Comment ***@***.***>
Subject: Re: [apache/xalan-java] Improve build: add CI, run xalan-test in CI, download jars from Central (PR #2)
wouldn't be the right answer for JLex, fetching https://www.cs.princeton.edu/~appel/modern/java/JLex/Archive/1.2.6/Main.java or whichever other specific release is desired.
Please read #2 (comment)<#2 (comment)>
Apparently, JLex.jar within xalan-java is modified, so there's no way to fetch a pre-built jar.
I would suggest integrating JLex in a source form, and building it during xalan build, however, it would be too many changes for the current PR which focuses on adding CI.
But it appears to be equivalent to the risk for java_cup.
xalan-java uses java-cup version 11b or something like that.
That version is not available on Central, so the only way to download it is to fetch from the project webpage and/or ask the maintainers to publish on Central.
—
Reply to this email directly, view it on GitHub<#2 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7OJ6WYZM27WJ6D5ABLQXTLXLFGWJANCNFSM54RHYSIA>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Frankly speaking, I do not find the question relevant to this PR. Whenever possible, I tried to avoid doing unnecessary modifications, so I selected the versions that were the same or close to the previously used versions. I believe, the existing java_cup was 11b, and I download 11b. What do you want to know by asking "tested with the newer java_cup release"? There's no "newer java_cup" release.
Would you please discuss JLex.jar patching in https://issues.apache.org/jira/browse/XALANJ-2635 ? |
This is resolved in the the migration from Ant to Maven, now in progress. Since JLex wasn't available in Maven Central, I went with JFlex instead, modifying the grammar to perform the lookahead via the regular expressions rather than by digging into the lex system's internal variables. Both necessary and cleaner, and quite possibly more performant though I haven't attempted to test that. |
The downloads aspects of this will be subsumed under the migration to Maven-based builds. Running CI looks useful. However, note that we are considering moving xalan-test into the test directories of xalan-java, so its invocation would change. |
Status check: This appears to be outdated, and to overlap with other work in progress. I believe we have already dealt with CI as a separate change.. I am dealing with bootclasspath. This was needed in earlier Javas because Sun insisted on shipping versions of of xerces and xalan in the standard libraries as org.apache.* (without repackaging them), preventing users from running the new code unless bootclasspath was prefixed or the -endorsedlib mechanism was used (which is essentially a more official version of the same thing). Since the introduction of JAXP and TrAX (circa Java 1.5-1.6), the java libraries have been changed to ship with the Apache code moved to com.sun.org.apache.*, removing that conflict. There may still be users who have the old workarounds in place so we should tolerate running in that mode (see recent discussion of Version)... but we don't need to use it ourselves. The Maven build prototype fixes most or all of the binary dependencies by downloading from Central. I expect to merge that soon. (Best practice is one issue per PR, though sometimes that issue unavoidably subsumes multiple tightly internlinked sub-issues, as is the case with the Maven migration uber-PR. Separation lets us discuss, refine, and approve them individually.) |
(Closing. Your more recent CI work is in other PRs, and I think the downloading-dependencies thing is going to be addressed by Maven cutover before too much longer so the Ant-based version, I think, is more distraction than useful. If you really feel it's needed as stopgap, please open a PR for that change separately and we can discuss.) |
This PR fixes build configuration, and removes many jars from the source repository and from the source distribution.
Here are the files that are still present:
Issue: https://issues.apache.org/jira/browse/XALANJ-2634
Sample CI log: https://github.com/vlsi/xalan-java/runs/7495309789?check_suite_focus=true#step:4:1