Skip to content

For LDEV-5331: fixed native library loading and error messages - #2

Closed
cfmitrah wants to merge 1 commit into
lucee:masterfrom
MitrahSoft:LDEV-5331
Closed

cfmitrah wants to merge 1 commit into
lucee:masterfrom
MitrahSoft:LDEV-5331

Conversation

@cfmitrah

Copy link
Copy Markdown
Member

No description provided.

@GianTschingt GianTschingt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for digging into this, the approach makes sense to me. I built the PR merged into current master locally: it compiles, and all eight native libs plus NativeLoader end up inside the extension jar as intended. The smaller fixes look right too (correct function name in the error, 1, 5 arg count, defaulting to ARGON2i instead of passing null to Argon2Factory.create()).

Two things I'd like changed before we merge:

  1. Stale or half-written lib in tmp. ensureLoaded() reuses java.io.tmpdir/lucee-argon2-native/<lib> whenever it exists and is non-empty. If the argon2 jar version changes, or two Lucee instances start at the same time and one is still writing, the other picks up an old or truncated library. Could you put the version in the folder name (e.g. lucee-argon2-native-2.12.0) and write to a temp file first, then Files.move(..., ATOMIC_MOVE)?
  2. A test. A small tests/ case that calls generateArgon2Hash() and argon2CheckHash() would let CI cover the loading path (labels argon2).

CI failed only at setup, because GPG secrets aren't available to fork PRs, so the build/test never ran. That will keep happening for fork PRs, so we'll run the tests on our side before merging. LDEV-5331 is linked.

@GianTschingt

Copy link
Copy Markdown
Contributor

No response for 10 days, so I'm closing this for now. Happy to reopen if anyone picks it up again.

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.

2 participants