Conversation
|
Could you please look at my review comments? Also, since you seem to know Appveyor, could you have a look at the two failing platforms? We would greatly appreciate any support on Windows. |
|
Thanks for the follow-up. I checked the PR conversation, review threads, and inline comments, but I cannot see your review comments yet. Could you link them or repost them so I can address the specific points? For Windows CI, PR build 1.0.1075 has two failing x86 XZ tests; |
| } | ||
|
|
||
|
|
||
| static int check_zero_length(void) { |
There was a problem hiding this comment.
Create one function that takes start and length as arguments. Copy expected into buffer, call _zip_crypto_clear() and check that the cleared range is 0 and the rest is unchanged.
There was a problem hiding this comment.
Addressed in b321748. The single check_range(start, length) helper copies the expected bytes, clears the requested span, and compares every byte against zero inside or its original value outside. It covers an interior range, zero length, and the full buffer. The focused CTest passes in native static, forced-fallback static, and native shared builds.
| #define ZIP_WANT_TORRENTZIP(za) ((za)->ch_flags & ZIP_AFL_WANT_TORRENTZIP) | ||
|
|
||
|
|
||
| #include <string.h> |
There was a problem hiding this comment.
We don't want to expose <string.h> unless we have to.
There was a problem hiding this comment.
Agreed, and addressed in b321748. zipint.h now declares the internal function without including <string.h>. The implementation includes it privately, and the source files that had relied on the transitive include now include the declarations they use directly. Local native and forced-fallback builds pass; Windows validation of this new head is pending.
| static inline | ||
| #endif | ||
| void | ||
| _zip_crypto_clear(void *buffer, size_t length) { |
There was a problem hiding this comment.
Why does this function have to be inline?
I think volatile is needed so the compiler optimizer doesn't remove accesses to it. Why can't we call memset on the volatile pointer?
There was a problem hiding this comment.
Inline was only needed for the old header-local fallback; b321748 moves clearing into a compiled internal function. memset accepts non-volatile void *, so passing a volatile pointer discards that qualifier, and casting it away does not prevent a dead-buffer memset from being optimized out. The fallback therefore keeps volatile byte stores; explicit_memset and explicit_bzero remain preferred where available. The internal symbol stays hidden, and the focused test compiles the same implementation directly. Local static and shared tests pass; Windows CI for this head is pending.
Sorry, my bad, I missed that I had to press the submit button at the top. They should be visible now. Thanks for investigating the CI build issues, we greatly appreciate it. |
Add defines for found functions in config.h Use explicit for loop over volatile pointer in fallback routine so compiler can’t opimize the clearing away if the buffer isn’t used afterwards. Based on PR #576.
|
We've merged your Your Appveyor commits seem like they add debug output. Can you run them on your branch? Or do we need to enable that, or should we commit them on the main branch? Thanks for looking into it. |
|
Yes. The temporary ARM failure diagnostics ran on this branch in AppVeyor 1.0.1098. They showed vcpkg ARM test-link failures (kernel32.lib / WindowsApp.lib) before libzip compilation; they did not fix those jobs. I removed the diagnostic hook and unsuccessful ARM overrides in 3490217, so there is no debug output left to enable or merge. The current branch ran again in 1.0.1101: x64 passed 193 tests; x86 failed only the two XZ conversion tests; ARM and ARM-UWP still failed before libzip. The remaining AppVeyor test-discovery changes are already on main via b2da382. No further AppVeyor commit is needed from this branch. |
|
Follow-up on the two x86 XZ failures: the controlled comparison now passes, with the same libzip
The five tests cover crypto clearing and both LZMA/XZ conversions; none are skipped. Source commit IDs, compiler metadata, generated test environments and JUnit output are retained in the run's This isolates the failing old-MSVC CLMUL path on this runner and matches XZ 5.8.2's documented compiler workaround. The vcpkg revision already used by your GitHub workflow contains liblzma 5.8.3, while AppVeyor 1.0.1108 used 5.8.1. Updating AppVeyor's dependency snapshot is therefore a concrete fix to validate next. This comparison does not establish a full AppVeyor matrix fix or resolve the separate ARM SDK/linker failures. I have not changed the existing PR branch. |
|
I pushed the AppVeyor dependency update and merged current main into the branch, keeping the integrated crypto implementation unchanged. The remaining PR diff is only seven added lines in The full AppVeyor build 1.0.1111 has finished:
This validates the dependency update against the actual x86 regressions; it is not a full ARM32 fix. No matrix entries or test conditions were removed or disabled. |
|
The ARM32 SDK fix is now validated on the actual AppVeyor workers. Build 1.0.1114, for head
The remaining diff against current main is |
This branch has not been deployed
2 participants
Summary
The build already probes for
explicit_memsetandexplicit_bzero, but their results were missing fromconfig.h.in. Propagate those results into the existing crypto-clearing backend selection.Implement
_zip_crypto_clearas a compiled internal function. It uses an explicit clearing API when available and volatile-qualified byte stores otherwise, so an ordinary dead-buffermemsetcannot be optimized away. The function remains hidden from the public API.zipint.hno longer exposes<string.h>; source files include the string declarations they use directly.The regression program uses one start/length helper that copies expected bytes, clears the requested range, and checks zero inside and unchanged bytes outside. It covers an interior range, zero length, and the full buffer. Because the symbol is hidden in shared builds, the test compiles the same implementation source directly.
AppVeyor installs nihtest with the Python 3.11 launcher already on its PATH, and test-enabled jobs fail if CTest discovers no tests. The first Windows run had compiled successfully while silently skipping regressions because unqualified
pyinstalled nihtest under Python 3.14.Validation
crypto_clear.test: pass in native static, forced-fallback static, and native shared builds, using nihtest 1.11.1.-O3 -Wall -Wextra -Werror. The fallback object retains the byte stores at-O3.<string.h>include, targeted strict compiles exposed undeclaredmemset/memcpyinzip_algorithm_xz.cand undeclaredmemsetin thezip_nonrandom.cregression helper. The current head adds direct includes there and inzip_crypto_openssl.c; all three affected sources pass focused declaration checks. This does not establish the cause of the Windows x86 failures.ac6ef11: x64 Windows ran 193 listed CTest cases with zero failures; x86 Windows passedcrypto_clear.testbut failed the two XZ conversion cases below. x64-UWP, x86-UWP, ARM64 Windows, and ARM64-UWP succeeded as compile-only lanes.b321748. Its x64 Windows test lane passed; x64-UWP failed during vcpkg zlib configuration and x86 Windows failed the two earlier XZ cases plus one LZMA case. The remaining lanes were still running or queued at 08:28 UTC. Build 1.0.1100 is for the intermediate7224aae. Build 1.0.1101 is queued for the current heade346cfa; it has no completed Windows validation yet.Windows CI gaps
19.29.30159.0and liblzma5.8.1. The two XZ conversion failures repeat across the prior heads; build 1.0.1099 also sawset_compression_store_to_lzma.testexit with Windows fast-fail0xC0000409. XZ 5.8.2's upstream change disables CLMUL CRC on older MSVC 32-bit x86 builds because optimized code can crash tests. The versions match that condition, but neither the XZ nor LZMA failures are causally attributed without a same-worker differential or crash subcode.10.0.19041.0, yet the linker cannot findkernel32.liborWindowsApp.lib, respectively. Unsuccessful SDK/triplet overrides were removed; no ARM worker fix is claimed.No failing tests have been disabled, and a passing full CI matrix is not claimed. The
explicit_memsetbackend has not been exercised here. This change strengthens the existing clearing primitive; it cannot guarantee erasure of every copy of sensitive data.Implementation and review were AI-assisted.