sshj icon indicating copy to clipboard operation
sshj copied to clipboard

Remove jzlib dependency

Open p120ph37 opened this issue 2 years ago • 4 comments

The JRE already contains a gzip implementation in java.util.zip.*, so we should be able to use that rather than relying on the external jCraft library.

p120ph37 avatar Mar 08 '22 02:03 p120ph37

Codecov Report

Merging #772 (ec74015) into master (dcfa183) will increase coverage by 0.01%. The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #772      +/-   ##
============================================
+ Coverage     64.88%   64.89%   +0.01%     
+ Complexity     1469     1465       -4     
============================================
  Files           210      210              
  Lines          8537     8523      -14     
  Branches        781      780       -1     
============================================
- Hits           5539     5531       -8     
+ Misses         2591     2582       -9     
- Partials        407      410       +3     
Impacted Files Coverage Δ
...zz/sshj/transport/compression/ZlibCompression.java 0.00% <0.00%> (ø)
...net/schmizz/sshj/transport/TransportException.java 44.44% <0.00%> (-11.12%) :arrow_down:
...ain/java/net/schmizz/sshj/common/SSHException.java 67.74% <0.00%> (-6.46%) :arrow_down:
src/main/java/net/schmizz/sshj/SSHClient.java 58.52% <0.00%> (-1.14%) :arrow_down:
...java/net/schmizz/sshj/transport/TransportImpl.java 64.87% <0.00%> (-0.83%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Mar 08 '22 06:03 codecov-commenter

Unfortunately I do not think this will work. Zip is different from zlib and both algorithms are unable to read eachother's data.

hierynomus avatar Mar 08 '22 06:03 hierynomus

Yes, a PKZip file is a different format from a Gzip file. But technically both use the "deflate" algorithm internally. Long ago, the JRE didn't contain tools for dealing directly with gzip/zlib style "deflate" streams (Java of course has supported access to PKZIP files since the beginning, since that's what .JARs are). As of Java 6 or so, (apparently as part of adding PNG support), classes were made available inside java.util.zip for handling "deflate" streams in a general-purpose way, compatible with gzip/zlib. See the docs here relating to the specific classes I used: https://docs.oracle.com/javase/6/docs/api/index.html?java/util/zip/package-summary.html Note it says "compression using the popular ZLIB compression library".

Additionally, I have tested this myself against an OpenSSH Docker container by doing this to force the use of my Zlib implementation (note the absence of the "NoneCompression.Factory"):

ssh.getTransport().getConfig().setCompressionFactories(Arrays.asList(
    new DelayedZlibCompression.Factory()
));

Give it a try -- it works for me 😁

p120ph37 avatar Mar 08 '22 19:03 p120ph37

I dug into the history a little more - it actually looks like zlib deflate/inflate support has been available in java.util.zip since all the way back in Java 1.1, when the JAR format was first introduced! https://www.cs.princeton.edu/courses/archive/fall97/cs461/jdkdocs/api/java.util.zip.Deflater.html

The only reason the jCraft jzlib project was even created was because the JRE implementation didn't yet support partial-flush mode, as explained here: http://www.jcraft.com/jzlib/

This deficiency was eventually fixed in Java 7 with the introduction of the 4-argument deflate(byte[] b, int length, int off, int flush), allowing the use of the SYNC_FLUSH flag: https://docs.oracle.com/javase/7/docs/api/java/util/zip/Deflater.html#deflate(byte[],%20int,%20int,%20int)

The Java bug report about this is here (and confirms that it was fixed as of JDK 7): https://bugs.java.com/bugdatabase/view_bug.do?bug_id=4206909

So I suppose if you are intending to still support Java 6 for now, maybe you can't merge this yet, but as soon as you are targeting Java 7 and above, this PR should be usable.

p120ph37 avatar Mar 10 '22 20:03 p120ph37

Is there a chance that this PR moves forward? I think that would be great! Just like jsch, jzlib seems to be unmaintained. And if jzlib can even be replaced by JDK internals, I think it's a good idea to do so.

Apache Mina Sshd did the same a while ago: https://github.com/apache/mina-sshd/commit/ea72a9eefa9872f6d41bd6c60a9a929824328051

hpoettker avatar Feb 10 '23 21:02 hpoettker

It looks like Java 7 may have already been a requirement since this commit: https://github.com/hierynomus/sshj/commit/35266945585b440ca75040d0af07181598531139#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R88

So I'm not aware of any reason this shouldn't be mergeable at this point. @hierynomus , do you have any remaining concerns?

p120ph37 avatar Feb 10 '23 21:02 p120ph37