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

Add Zlib Library For Compression Support #361

Closed
wants to merge 2 commits into from

Conversation

NoMoreFood
Copy link

  • Added an empty project with a script to dynamically download zlib source and compile it into a static library.
  • Updated other projects to link with new zlib library.
  • Removed zlib preprocessor workarounds and function stubs now that real library is in place.

- Added an empty project with a script to dynamically download zlib source and compile it into a static library.
- Updated other projects to link with new zlib library.
- Removed zlib preprocessor workarounds and function stubs now that real library is in place.
@manojampalam
Copy link

@NoMoreFood great work.

I'll check with folks on our side if we could get this into Windows.

In the man time, can you move zlib into its own repo as its independent of OpenSSH - You could publish zlib libraries for Windows in that repo and have this fork consume it.

@NoMoreFood
Copy link
Author

@manojampalam Do you think a separate repo is warranted for this? This pull does not actually commit the put the zlib code into the OpenSSH repo -- it simply references it by a URL to the main zlib site that we can easily change if it updates; most of the file changes are actually to the other project files to link in the dependency library. This pull request includes a stub project file and downloads/compiles the zlib code as a static library on-the-fly. Because it's integrated in this way, it's easy to build for matching compiler runtime library (VS2015, VS2017), configuration (Debug, Release) and architecture (ARM, ARM64, x86, x64). To do a proper library repo, we'd need to copy/maintain a separate set of libraries for the aforementioned permutations (16 total if you do all the ones I mentioned). Thoughts?

@manojampalam
Copy link

That's exactly how we would like to do it , if we were to take the support upstream to Windows fork() - Maintain and service zlib from a different independent repository.

But hang on before you do further changes, I initiated talks internally on where we should take this. Will update you on where it goes and what our next steps would be.

@NoMoreFood
Copy link
Author

@manojampalam Sounds good. Let me know what you decide.

@NoMoreFood
Copy link
Author

@manojampalam Any update on this? I'll go ahead and make the separate repository if that's the route you want to go.

@manojampalam
Copy link

@NoMoreFood I'm working to have a repo setup under Powershell project for zlib. (similar to LibreSSL at https://github.com/PowerShell/LibreSSL).

@manojampalam
Copy link

manojampalam commented Jan 21, 2019

@NoMoreFood I've put 1.2.11 sources in https://github.com/PowerShell/ZLib. Plan is to have zlib SDK and binaries published here. We'll pull SDK and compile into OpenSSH (similar to what we do for LibreSSL or better).
Whether to statically link all code or have a DLL for zlib is something we need to decide.

We could start off with statically linking all code, as I believe it would only pull in required zlib code into OpenSSH binaries (ssh.exe and sshd.exe). What increase in binary size did you observe?
We could also explore creating a dll with only the required exports (I believe we only need 6 functions).

@NoMoreFood
Copy link
Author

@manojampalam I'll let you know on the sizes this weekend. There also may be some runtime memory considerations since that the linker should have to include less of the object code given it knows which functions are being used at compile time (and can optimize accordingly). If the library is going to be unconditionally loaded at runtime, this could result in less RAM usage when compared to using a DLL although it may of course lead to increased executable sizes. The same would apply to LibreSSL. We also avoid library path lookup concerns and library hijacking exploits.

@NoMoreFood
Copy link
Author

NoMoreFood commented Feb 4, 2019

@manojampalam See below. Results as expected. The static library produces executables that require less RAM but have larger executable sizes. If you count the dynamic library size, it actually has a larger footprint on the file system. I suspect libcrypto would have similar results. Since reuse of libcrypto.dll and zlib.dll beyond these executables is probably unlikely, I'd recommend making both static. Startup time and performance may also be improved since the process loader does not need to locate another file on the disk and more linker optimizations are possible with static libraries.

File System Sizes

File Static Compile Dynamic Compile
ssh-keyscan.exe 577 KiB 533 KiB
ssh.exe 936 KiB 892 KiB
sshd.exe 1047 KiB 1004 KiB
zlib.dll N/A 195 KiB
Total 2560 KiB 2624 KiB

Runtime Memory Usage

File Static Compile Dynamic Compile
ssh.exe 1340 KiB 1384 KiB
sshd.exe 1184 KiB 1240 KiB
Total 2524 KiB 2624 KiB

Runtime usage was taken by sampling the private working set memory usage in Task Manager immediately after the service was started (for sshd.exe) and after a connection was made (for ssh.exe).

@lars18th
Copy link

Hi @manojampalam ,

Are you considering integrating this PR soon?

@manojampalam
Copy link

@lars18th not yet. we have some work on our side to put in place an official build pipeline for the new repo.

@WSLUser
Copy link

WSLUser commented Jul 24, 2019

Status update?

@lars18th
Copy link

I hope this will be merged soon.
Thank you @NoMoreFood ! 👍

@WSLUser
Copy link

WSLUser commented Sep 5, 2019

Ping. @manojampalam

@bagajjal
Copy link

bagajjal commented Sep 6, 2019

We are working on it.

@JustinGrote
Copy link

LGTM! I have a usecase (tmate) that only supports zlib, so I can't use this for that connection, I've been using nomorefood's compiled branch version.

@bagajjal
Copy link

@NoMoreFood - Thanks for your work.
I am working on integrating the Zlib with OpenSSH.

He is my approach (similar to Libressl),

  1. As discussed above, maintain a separate repo https://github.com/PowerShell/ZLib
  2. Build and release the ZLib binaries in it's own repo.
  3. Before building the OpenSSH, get the latest ZLib binaries.
  4. Use static linking as there is no much increase in the binary size.

I am closing this PR. I will create a new PR.

@bagajjal bagajjal closed this Oct 11, 2019
@NoMoreFood
Copy link
Author

@bagajjal Sounds good. Other than any necessary project file changes and the code to download the binaries, also remember to address the changes I made in 'https://github.com/PowerShell/openssh-portable/pull/361/files#diff-a291845448c010641b66873a8340f7c1' and 'https://github.com/PowerShell/openssh-portable/pull/361/files#diff-a291845448c010641b66873a8340f7c1'. Also to remove 'win32_zlib.c'.

@bagajjal
Copy link

@NoMoreFood - I am comparing project file created by you vs the VSProj in "$\zlib-1.2.11\contrib\vstudio\vc14\zlibstat.vcxproj".

I have a few questions,

  1. There are multiple configurations (one with ASM, without ASM) i.e, Release vs ReleaseWithoutAsm.
    I am NOT planning to take ASM. Do you know if this has any side effects? I am guessing this might slow the compression / decompression. Is it right?

  2. The zlib vsproj ("$\zlib-1.2.11\contrib\vstudio\vc14\zlibstat.vcxproj".) uses $\zlib-1.2.11\contrib\minizip\zip.c, $\zlib-1.2.11\contrib\minizip\unzip.c and $\zlib-1.2.11\contrib\minizip\ioapi.c

I don't see this in your project file. Do we need zip.c, unzip.c and ioapi.c?

  1. ZLIB_WINAPI is used in "$\zlib-1.2.11\contrib\vstudio\vc14\zlibstat.vcxproj". It's unclear if this is required.
    The binary (without ZLIB_WINAPI) on win7 works perfectly fine.

@NoMoreFood
Copy link
Author

  1. Correct; presumably the ASM compilation could be marginally faster but the difference is likely not worth the pain of integrating the assembler.

  2. You do not need those files. Minizip is an executable that's built using zlib; it is not important when compiling zlib support into OpenSSH.

  3. Topically it looks like ZLIB_WINAPI may only have value when doing a dynamic library compile.

@NoMoreFood NoMoreFood deleted the zlib_embedded branch August 26, 2022 10:22
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.

6 participants