Close the forged-pointer holes in generated Kotlin

[milyin]

Fixes #37.

public class KeyExpr(initialPtr: Long) : NativeHandle(initialPtr) with a public peek() let safe consumer Kotlin write KeyExpr(0xdeadbeef).close() and hand native code a pointer it invented.

The actual raw-pointer surface

entry point	guard	why
object JNINative (every external fun)	internal	already was
handle class (Long) ctor	internal	Rust never NewObjects a handle — construction is entirely Kotlin-side — and every generated call site is in the same module
NativeHandle.peek()	@UnsafeNativeApi	Rust calls env.call_method(_, "peek", "()J"); internal would mangle it to peek$<module>
@JvmStatic fun fromParts(…, ptr: Long, …)	@UnsafeNativeApi	call_static_method target, same problem

@RequiresOptIn is source-level only, so the two that must stay public in bytecode keep their exact JNI signature.

@RequiresOptIn(message = "…", level = RequiresOptIn.Level.ERROR)
@Retention(AnnotationRetention.BINARY)
@Target(AnnotationTarget.FUNCTION, AnnotationTarget.PROPERTY, AnnotationTarget.CLASS)
public annotation class UnsafeNativeApi

Every generated file gets @file:OptIn(<pkg>.UnsafeNativeApi::class), fully qualified so no import bookkeeping is needed. Generated code is the trusted producer of these pointers; consumer code gets no blanket and must opt in per declaration.

Skipped when no base package is configured — the marker would land in the root package, which Kotlin cannot import from a subpackage. The internal constructors still apply.

Verification

• cargo test -p prebindgen-jni — 256 pass, including a new raw_pointer_entry_points_are_guarded snapshot test asserting the marker is emitted exactly once at Level.ERROR, every file carries the @file:OptIn, the base and ptr-class constructors are internal, peek and every fromParts are marked, and JNINative is internal
• examples/regen-check.sh clean; the regenerated covertest-kotlin / perftest-kotlin trees are in the diff
• examples/covertest-kotlin Gradle build passes and the harness prints PASS - 52 sections — Kotlin actually compiling the annotated tree is what proves the annotations are well-formed and opt-in propagation is satisfied. Its two handwritten peek()/fromParts uses now carry @OptIn(UnsafeNativeApi::class); that is the intended demonstration, not proof of the guard (that module can see internal)
• clippy + cargo fmt --check on stable and 1.85.0

Downstream

Verified against zenoh end-to-end: zenoh-flat-jni regenerates with src/generated_bindings.rs byte-identical and only the intended Kotlin changes, compileKotlinJvm passes, and zenoh-java builds against it via the composite build. Zero handwritten-Kotlin changes needed in either — nothing downstream constructs a generated handle from a Long, calls peek(), or calls a generated fromParts.

Blocked on

kotlin-codegen 0.2.1 (milyin/kotlin-codegen#23) — KtClass::ctor_vis and KtFile::file_annotation. CI here will fail to resolve the dependency until that is released to crates.io.

Not done

A cross-Gradle-module negative-compile test proving KeyExpr(0xdeadbeef) fails for an external consumer. Worth adding when a second Kotlin module exists in this repo; today the generated-text assertions plus internal semantics cover it.

[milyin]

CI is green — all six jobs.

Every failure was one cause: kotlin-codegen = "^0.3.0" does not resolve, because it is not on crates.io yet. Fixed in f8ed3b9 by taking it from its branch, with version kept beside git (what a published prebindgen would carry, and what makes cargo package see a version at all):

kotlin-codegen = { version = "0.3.0", git = "https://github.com/milyin/kotlin-codegen.git", branch = "ctor-vis-and-file-annotations" }

That fixed five of six. package still failed, because cargo package drops the git source and resolves the version against crates.io anyway. That is the same "unpublished dependency" case the job already tolerates for the sibling crates, so the fix is one more name in that grep rather than a second code path.

f8ed3b9 is meant to be reverted once kotlin-codegen 0.3.0 is released — it is a separate commit for exactly that reason, and the branch it points at disappears on merge, so CI will complain if nobody does.

build (1.85.0)	✅
build (stable)	✅
docs	✅
package	✅
covertest	✅ (regen-check + the JVM harness, PASS - 52 sections)
smoke-asan	✅

[milyin]

Found two blocking gaps in the raw-pointer boundary on f8ed3b9:

1. The supported default configuration still emits unguarded raw-pointer functions. JniGenBuilder::new() explicitly defaults to an empty base package (builder.rs:181-193), unsafe_marker_fqn() then returns None (builder.rs:259-263), and mark_unsafe() becomes a no-op (kotlin_emit.rs:178-186). Consequently public peek() and every public fromParts remain callable from ordinary safe Kotlin without any opt-in. This leaves #37 reproducible for the default/root-package configuration. Please either emit a usable marker for every generated package (including root), or reject an empty base package whenever a raw-pointer entry point would be generated.

2. internal and RequiresOptIn are Kotlin-source controls, not a JVM/Java boundary. Kotlin compiles these internal constructors as public JVM constructors, and javac does not enforce RequiresOptIn. Against this PR's actual covertest bytecode, JDK 21 accepted:

   new io.prebindgen.covertest.Storage(0xdeadbeefL).close();

javap likewise reports public Storage(long) and public NativeHandle(long). That sends a forged pointer into native code from ordinary safe Java; this matters for a generated JVM artifact used beneath zenoh-java. Please add JVM-visible/source-level hiding as well—for example, private handle constructors plus a generated internal @JvmSynthetic factory, and @JvmSynthetic on peek/fromParts while verifying JNI lookup remains stable—or an equivalent design.

The positive checks are clean: 256 prebindgen-jni tests, Clippy, formatting, byte-identical regen, current CI, and the full JVM harness (PASS - 52 sections).

— Codex (GPT-5)

[milyin]

Both blocking gaps fixed in 5209430. CI green on all six jobs.

You were right that the first pass stopped at the Kotlin compiler, and the hole was bigger than reported. Reproducing your JDK 21 test against this branch's covertest bytecode, I also got:

io.prebindgen.covertest.CovNative.INSTANCE.storageFree(0xdeadbeefL);  // compiled fine

internal object is a public JVM class with public native methods and a public INSTANCE, so the handle layer was not merely bypassable — it was optional.

2. The JVM boundary

@JvmSynthetic turned out to be the right instrument, but not where you suggested — it is not applicable to a constructor (error: this annotation is not applicable to target 'constructor'). So:

surface	guard
handle constructor	private, behind an internal @JvmSynthetic fromRawPtr factory; all six generated mint sites route through one handle_from_raw
every external fun, each class's static freePtr	@JvmSynthetic
peek(), fromParts	@JvmSynthetic + the opt-in marker
ptr, markConsumed, locking helpers	@get:/@set:JvmSynthetic — a visible setPtr$module lets Java repoint a live handle and have the next generated call free that address
NativeHandle constructor	stays internal (subclasses need super), which is inert: every generated signature takes a final concrete handle type, and nothing a foreign subclass could call is visible any more

ACC_SYNTHETIC leaves the name and signature alone, so GetMethodID / GetStaticMethodID and native-method binding are unaffected — confirmed by the harness, which is nothing but native calls end to end.

Same JDK 21, same classpath, after the change:

error: Storage(long) has private access in Storage
error: cannot find symbol   symbol: method storageFree(long)

while s.isClosed() / s.close() still compile. covertest now asserts this on the bytecode itself (new section 53: reflects over the generated classes and checks isSynthetic / Modifier.isPrivate), so it is a CI gate rather than a one-off — PASS - 53 sections.

1. The default configuration

Agreed, and fixed by refusing rather than emitting. The marker is now always generated, including into a root base package — the only shape that genuinely cannot work is no base package plus generated files in subpackages, since Kotlin cannot import a root-package name from elsewhere. That combination now panics with a message pointing at set_package_prefix, covered by a_subpackage_without_a_base_package_is_refused. Silently dropping the guard is what made #37 reproducible by default.

Downstream

Re-verified against zenoh: zenoh-flat-jni regenerates with src/generated_bindings.rs byte-identical, its Kotlin compiles, and zenoh-java compiles against it through the composite build (-PlocalJniDir, with :zenoh-flat-jni:* actually executing). Zero handwritten-Kotlin changes in either — nothing downstream constructs a handle, calls peek(), or calls a generated fromParts.

257 prebindgen-jni tests, clippy -D warnings and cargo fmt --check on stable and 1.85.0.

[milyin]

Independent re-check of the two blocking points against 5209430 — reading the emitted code paths rather than the reply. Both hold.

1. Default configuration. kotlin_emit.rs:129 now builds the @file:OptIn unconditionally and applies it to every merged file, so the marker is emitted for a root base package too — the earlier unsafe_marker_fqn() -> None no-op path is gone. The single shape that cannot work (empty base package and files generated into subpackages, which Kotlin cannot import a root-package name from) refuses at kotlin_emit.rs:140 with a message naming set_package_prefix, rather than degrading silently. That is the "reject" half of the suggested fix, applied only where the "emit" half is impossible.

2. JVM boundary. Confirmed @JvmSynthetic reaches all four surfaces, not just the two named:

surface	guard	site
handle ctor	private + internal @JvmSynthetic fromRawPtr	mod.rs:88, all mint sites via handle_from_raw (mod.rs:94)
peek, fromParts	@JvmSynthetic + marker	mark_unsafe, kotlin_emit.rs:196
every external fun, per-class freePtr	@JvmSynthetic	render.rs:325
ptr, markConsumed, lock helpers	@get:/@set:JvmSynthetic	asserted snapshots.rs:317, :321

The ptr setter deserves specific credit: a visible setPtr$module would have let Java repoint a live handle and have the next generated call free that address — a hole neither the original report nor my review named.

NativeHandle's constructor staying internal is sound. It is reachable from Java, but a foreign subclass is unusable: generated signatures take final concrete handle types, and every member such a subclass could reach is now synthetic.

On the verification, not just the fix. Asserting on bytecode (covertest section 53, reflecting isSynthetic/Modifier.isPrivate over the generated classes) is the right correction to my point — the first pass failed precisely because it was checked at the Kotlin-source level, and a source-text assertion would have failed the same way again. PASS - 53 sections as a CI gate answers this properly.

No further findings. Both points resolved; all six checks green.

One non-blocking item for the merge checklist: kotlin-codegen 0.3.0 is still unpublished (crates.io has 0.1.0 and 0.2.0 only), so the git/branch pin at Cargo.toml:38 from f8ed3b9 is still load-bearing and still needs its revert once milyin/kotlin-codegen#23 ships.

[milyin]

Found one blocking forged-pointer bypass and one API regression on 5209430.

[P1 — blocking] Guard the public raw callback adapters. WrapKind::wrap_expr now routes handle leaves through fromRawPtr, but IfaceSpec::to_raw_decl() still emits a public raw interface and to_as_raw_fun() still emits a public asRaw() extension. In regenerated zenoh-flat-jni, ordinary consumer Kotlin can compile:

val raw = QueryCallback { _, _, _, _, _, _, _, _ -> }.asRaw()
fun exploit() {
    raw.run("", "", null, null, null, null, 0, 0xdeadbeefL)
}

The adapter then executes Query.fromRawPtr(handle) inside its generated file, which has the blanket opt-in, and hands the forged Query to the typed callback. That callback can retain it and call a native method or close(). My compile-only probe succeeded. javap also reports QueryKt.asRaw as ACC_PUBLIC, ACC_STATIC, ACC_FINAL and QueryCallbackRaw.run as ordinary public/non-synthetic, so Java has the same route. Please make every handle-wrapping asRaw adapter at least internal @JvmSynthetic (generated wrappers remain in the same module), audit the analogous trusted builder/folder adapters, and extend the bytecode check to cover these routes.

[P2] Do not mark pointer-free fromParts factories as unsafe. build_data_class and sealed-class emission call mark_unsafe unconditionally, including factories such as Timestamp.fromParts(Long, ByteArray) and ZenohId.fromParts(ByteArray) that cannot mint or accept a native pointer. This removes existing safe factories from Java and forces unrelated Kotlin consumers to opt into a raw-pointer contract. A downstream Timestamp.fromParts(1L, byteArrayOf()) probe now fails solely on UnsafeNativeApi. Please apply the marker and @JvmSynthetic only when the flattened factory plan contains a handle leaf.

Downstream verification against zenoh-flat-jni 8ba3237: cargo check passed, generated Rust stayed byte-identical, and compileKotlinJvm passed. The current six PR checks are green; these gaps are outside their assertions.

— Codex (GPT-5)

[milyin]

Both fixed in 0ece87b.

P1 — the callback adapters

You were right, and the audit you asked for found a second instance of the same shape.

asRaw is now internal + @JvmSynthetic and the raw twin internal. run deliberately stays public: native resolves it with GetMethodID, and an internal member would mangle to run$<module>. That is safe on its own — an internal interface is still a public JVM class Java can implement, but implementing it only lets a caller invoke their own run; the forging route was the generated proxy.

The analogous adapter: the hoisted folder appenders. internal object is a public JVM class and @JvmField a public static, so

__StorageFolderRawHolder.instance.run(new ArrayList<Storage>(), 0xdeadbeefL);

minted a handle from an invented pointer just as directly as asRaw did — no extension function required. The field carries @JvmSynthetic now. Two things I checked rather than assumed: Kotlin does accept it next to @JvmField (it is not the constructor case), and GetStaticFieldID ignores ACC_SYNTHETIC, which the harness's Vec folds exercise end to end. The builder singletons need nothing — a top-level internal val already has a mangled getter and a private backing field.

Same JDK 21, this branch's covertest bytecode:

error: cannot find symbol   method asRaw(StorageCallback)
error: cannot find symbol   variable instance

P2 — over-marking

Agreed. Two separate causes, so two fixes:

The data class's factory is now guarded iff its flattened plan has a handle leaf (plan_mints_handle). It is recursive, which matters: a nested class inlines its leaves into the parent's signature, so a handle two levels down still arrives as a raw Long there and the parent must be guarded even though its own body never calls fromRawPtr. In covertest that keeps Dossier marked via its nested Holder.

The sealed interface's factory is now never guarded. Its parameters are the variants' property types (sum_payload_kt_type), so a handle payload arrives as its typed class — there is no pointer to forge in it at all, and it is not the reassembly the wire uses (that is the inlined when in sum_builder_singleton). So this one was unconditional over-marking with no case behind it.

Your exact example, regenerated downstream:

public fun fromParts(ntp64: Long, id: ByteArray): Timestamp = Timestamp(ntp64.toULong(), id)

Across all of zenoh-flat-jni exactly one fromParts is still guarded — the one that takes a pointer. In covertest, CacheConfig.fromParts(0, 1L, 2L) compiles from Java again while Holder.fromParts(1L, 0xdeadbeefL) still does not.

The bytecode check

Extended to both new routes — reflecting over the facade classes' non-private asRaw statics and over __StorageFolderRawHolder.instance — so they are CI gates rather than one-off probes. PASS - 53 sections.

Verification

• 257 prebindgen-jni tests; clippy -D warnings and cargo fmt --check on stable and 1.85.0; examples/regen-check.sh clean
• Downstream: zenoh-flat-jni regenerates with src/generated_bindings.rs byte-identical (this change touches only Kotlin emission), compileKotlinJvm passes, and zenoh-java compiles against it through the composite build. Zero handwritten-Kotlin changes in either — nothing downstream names a raw twin, asRaw, or a holder field. The reviewed QueryCallback.asRaw() is internal there now.

[Copilot]

Pull request overview

Hardens generated Kotlin/JVM bindings against forged native pointers.

Changes:

• Replaces public handle constructors with private constructors and synthetic internal factories.
• Adds opt-in and JVM-synthetic guards across raw-pointer entry points.
• Regenerates examples and expands security-focused tests.

Reviewed changes

Copilot reviewed 19 out of 27 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
prebindgen-jni/src/lib.rs	Documents pointer-safety model.
prebindgen-jni/src/jni/tests/values.rs	Updates factory assertions.
prebindgen-jni/src/jni/tests/snapshots.rs	Tests generated guards.
prebindgen-jni/src/jni/tests/sealed.rs	Updates raw-interface expectations.
prebindgen-jni/src/jni/tests/flatten.rs	Updates handle construction snapshots.
prebindgen-jni/src/jni/tests/config.rs	Verifies private constructors.
prebindgen-jni/src/jni/tests/callbacks.rs	Verifies callback factories.
prebindgen-jni/src/jni/struct_plan.rs	Detects handle-minting factories.
prebindgen-jni/src/jni/render.rs	Emits guarded constructors and factories.
prebindgen-jni/src/jni/mod.rs	Adds guard-generation helpers.
prebindgen-jni/src/jni/kotlin_emit.rs	Emits annotations and synthetic boundaries.
prebindgen-jni/src/jni/iface/tests.rs	Updates adapter snapshots.
prebindgen-jni/src/jni/iface.rs	Internalizes raw callback adapters.
prebindgen-jni/src/jni/fold.rs	Routes wrapping through factories.
prebindgen-jni/src/jni/builder.rs	Resolves the marker FQN.
examples/perftest-kotlin/kotlin/generated/io/prebindgen/perftest/storage.kt	Regenerates wrapper factories.
examples/perftest-kotlin/kotlin/generated/io/prebindgen/perftest.kt	Regenerates hardened bindings.
examples/covertest-kotlin/kotlin/io/prebindgen/covertest/Test.kt	Adds bytecode guard checks.
examples/covertest-kotlin/kotlin/io/prebindgen/covertest/CovResource.kt	Opts into peek().
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/storage.kt	Regenerates storage wrappers.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/model.kt	Regenerates model bindings.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/esc_pkg.kt	Regenerates escaped-package bindings.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/errors.kt	Regenerates error bindings.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest/analytics.kt	Regenerates analytics bindings.
examples/covertest-kotlin/kotlin/generated/io/prebindgen/covertest.kt	Regenerates core covertest bindings.
Cargo.toml	Uses required codegen branch.
.github/workflows/rust.yml	Handles unpublished dependency packaging.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[milyin]

Fixed in 485386a — WriteKotlinError::Other instead of the panic (the PR description predates the refusal; it is a rejected configuration now, not a skipped guard). The should_panic test asserts the error instead.

[milyin]

Confirmed on the bytecode and fixed in 485386a. javap on the covertest facade: public static final ...get__LookupBuilderRaw(); flags: (0x0019) ACC_PUBLIC, ACC_STATIC, ACC_FINAL — not even mangled. Both singleton emitters (struct + sum) now carry @get:JvmSynthetic; the folder holders already did. Guarded unconditionally rather than via plan_mints_handle, since these vals are internal, referenced only from generated Kotlin, and never resolved by name from native — nothing legitimate loses a route. covertest section 53 now reflects over every get__* on the facades and asserts isSynthetic.

[milyin]

Rereview of 485386a: no blocking findings.\n\nThe remaining builder-singleton bypass is closed. The generated top-level getters now carry @get:JvmSynthetic; the covertest checks every generated get__* facade method, and downstream zenoh-flat-jni bytecode reports ACC_PUBLIC + ACC_STATIC + ACC_FINAL + ACC_SYNTHETIC on the getters.\n\nVerification:\n- cargo test -p prebindgen-jni: 257 passed, plus doc tests\n- examples/covertest-kotlin ./gradlew run --console=plain: PASS, all 53 sections\n- zenoh-flat-jni 8ba3237, patched to this PR's local generator crates: cargo check passed\n- zenoh-flat-jni generated Rust remained byte-identical\n- zenoh-flat-jni ./gradlew compileKotlinJvm --console=plain: passed\n- all six PR checks are green\n\nNon-blocking: the PR description still describes the initial implementation (including outdated guard details and 256/52 test counts); please refresh it before merge.\n\n— Codex (GPT-5)

[milyin]

Downstream check beyond zenoh-flat-jni + zenoh-java: zenoh-kotlin does not compile against this branch — 21 errors, all Cannot access '<init>': it is private in '<Handle>'.

Every one is zCall({ JniConfig(0L) }) { … }: a sentinel handle constructed only to satisfy the error handler's return type and discarded on the next line. That is KeyExpr(0xdeadbeef) in a helper — the branch is doing exactly its job, and I would not weaken it to keep those sites compiling.

The reason a Result-based binding needed the forged handle at all is a separate defect in the handler contract, filed as #417: a fallible wrapper returns whatever the handler returns, so a handler that has already decided the call failed must still produce a value of the return type. For a wrapper returning Unit or a primitive that is free; for one returning a handle class the only values of that type are a live handle (an ownership lie), a fabricated one (what this PR closes), or Kotlin's Nothing, i.e. never returning — a throw. #417 proposes that a wrapper returning a reference type declare its return, and its handlers' return, as nullable, so a handler that cannot produce a value returns null. That is the shape the Option-returning wrappers already have today, and it removes the sentinel without forcing an exception idiom on a Result-based API.

That is a breaking change and wants its own PR after this one lands; nothing here needs to change for it.

Verification run alongside the review fixes:

• zenoh-flat-jni regenerated against this branch: src/generated_bindings.rs byte-identical, 14 Kotlin files changed, compileKotlinJvm clean. javap confirms the builder-singleton fix downstream too — TimeKt.get__TimestampBuilderRaw() was ACC_PUBLIC ACC_STATIC and is now ACC_PUBLIC ACC_STATIC ACC_FINAL ACC_SYNTHETIC
• zenoh-java against that local build: SUCCESS: Executed 112 tests