Incorrect mapping of c.s.j.platform.unix.solaris.LibKstat.KstatCtl

Commence dans com/sun/jna/platform/unix/solaris/LibKstat.java : le champ KstatCtl.kc_chain doit devenir Pointer avec un accesseur chain(), et Kstat a besoin de constructeurs explicites sans argument et avec Pointer (en miroir de KstatNamed/KstatIO dans le même fichier) afin que next() puisse abandonner sa paire useMemory/read. Compile le reproducer Probe de l'issue pour confirmer que sizeof(KstatCtl) passe de 200 à 24 avant et après l'édition. Terminé signifie que le layout de la struct correspond à <kstat.h>, qu'aucun sur-lecture/sur-écriture de champ ne subsiste, et que les maintainers se sont accordés sur le plan déprecation-vs-rename pour le changement de type du champ public.

LibKstat.KstatCtl declares the kstat chain as an inline Structure where C declares a pointer to one:

@FieldOrder({"kc_chain_id", "kc_chain", "kc_kd"})
class KstatCtl extends Structure {
    public int kc_chain_id;
    public Kstat kc_chain;   // real type: kstat_t *
    public int kc_kd;
}

The real type, from <kstat.h> (identical on illumos and Oracle Solaris):

typedef struct kstat_ctl {
    kid_t    kc_chain_id;
    kstat_t *kc_chain;
    int      kc_kd;
} kstat_ctl_t;

JNA computes sizeof(kstat_ctl_t) as 200 bytes, where the real struct is 24:

sizeof(JNA KstatCtl) = 200   [real kstat_ctl_t = 24]
    kc_chain_id @ 0
    kc_chain    @ 8   (184 bytes)
    kc_kd       @ 192

Symptom: heap corruption

kstat_open() returns a pointer to a 24-byte block it allocated. JNA wraps that pointer in a KstatCtl and autoRead()s it (Function.java:450), reading 176 bytes past the end. Worse, a Structure passed by reference calls autoWrite() before every call (Function.java:535), so each

• kstat_chain_update(kc)
• kstat_lookup(kc, …)
• kstat_read(kc, ksp, …)
• kstat_close(kc)

writes 176 bytes past the end of that 24-byte allocation.

The payload is the snapshot captured by the previous call's autoRead, so the effect is to revert 176 bytes of neighboring heap to their contents at the time of the last kstat call. A caller that keeps one kstat_ctl_t open for the process lifetime (the documented usage) reverts that window on every query, undoing arbitrary amounts of unrelated allocator activity.

Observed impact

OSHI hit this as an intermittent JVM crash on OpenIndiana at roughly a 15% rate per CI run. It only became diagnosable after the test runs were switched to libumem (LD_PRELOAD_64=libumem.so.1); under libc malloc the same corruption had been silently producing unexplained faults (at a much lower rate).

Reproducer

The defect is in JNA's computed layout, so it can be measured on any platform by mirroring the declarations (instantiating LibKstat.KstatCtl directly triggers Native.load("kstat") in the interface's static initializer, so the mirror is needed off-Solaris):

import com.sun.jna.*;
import com.sun.jna.Structure.FieldOrder;

public class Probe {
    static final int KSTAT_STRLEN = 31;

    // Verbatim copy of LibKstat.Kstat's field declarations.
    @FieldOrder({ "ks_crtime", "ks_next", "ks_kid", "ks_module", "ks_resv", "ks_instance", "ks_name",
            "ks_type", "ks_class", "ks_flags", "ks_data", "ks_ndata", "ks_data_size", "ks_snaptime",
            "ks_update", "ks_private", "ks_snapshot", "ks_lock" })
    public static class Kstat extends Structure {
        public long ks_crtime;
        public Pointer ks_next;
        public int ks_kid;
        public byte[] ks_module = new byte[KSTAT_STRLEN];
        public byte ks_resv;
        public int ks_instance;
        public byte[] ks_name = new byte[KSTAT_STRLEN];
        public byte ks_type;
        public byte[] ks_class = new byte[KSTAT_STRLEN];
        public byte ks_flags;
        public Pointer ks_data;
        public int ks_ndata;
        public long ks_data_size;
        public long ks_snaptime;
        public int ks_update;
        public Pointer ks_private;
        public int ks_snapshot;
        public Pointer ks_lock;
    }

    // Verbatim copy of LibKstat.KstatCtl's field declarations.
    @FieldOrder({ "kc_chain_id", "kc_chain", "kc_kd" })
    public static class KstatCtl extends Structure {
        public int kc_chain_id;
        public Kstat kc_chain;
        public int kc_kd;

        void dump() {
            System.out.println("sizeof(KstatCtl) = " + size() + "  (real kstat_ctl_t = 24)");
            System.out.println("  kc_chain_id @ " + fieldOffset("kc_chain_id"));
            System.out.println("  kc_chain    @ " + fieldOffset("kc_chain"));
            System.out.println("  kc_kd       @ " + fieldOffset("kc_kd"));
        }
    }

    public static void main(String[] args) {
        new KstatCtl().dump();
    }
}

Output on JNA 5.19.1, x86-64 / aarch64 LP64:

sizeof(KstatCtl) = 200  (real kstat_ctl_t = 24)
  kc_chain_id @ 0
  kc_chain    @ 8
  kc_kd       @ 192

Suggested fix:

 @FieldOrder({"kc_chain_id", "kc_chain", "kc_kd"})
 class KstatCtl extends Structure {

     public int kc_chain_id; // current kstat chain ID

-    public Kstat kc_chain; // pointer to kstat chain
+    public Pointer kc_chain; // pointer to kstat chain

     public int kc_kd; // /dev/kstat descriptor - not public interface
+
+    public Kstat chain() {
+        if (kc_chain == null) {
+            return null;
+        }
+        return new Kstat(kc_chain);
+    }
 }

Kstat needs a pointer constructor for that, which KstatNamed and KstatIO in the same file already have.
Adding one means declaring the no-arg constructor explicitly, since it is currently implicit, and it lets
next() drop its hand-rolled useMemory/read pair:

 class Kstat extends Structure {
     …
     public Pointer ks_lock; // protects this kstat's data

+    public Kstat() {
+        super();
+    }
+
+    public Kstat(Pointer p) {
+        super(p);
+        read();
+    }
+
     public Kstat next() {
         if (ks_next == null) {
             return null;
         }
-        Kstat n = new Kstat();
-        n.useMemory(ks_next);
-        n.read();
-        return n;
+        return new Kstat(ks_next);
     }
 }

Structure(Pointer) attaches the memory without reading it, which is why both constructors above call read()
explicitly, matching KstatNamed. The field initializers for ks_module, ks_name and ks_class run between
super(p) and the constructor body, so the arrays exist by the time read() populates them.

As a side benefit, Structure.newInstance(Kstat.class, ptr) picks up a public single-Pointer constructor when
one exists (getPointerConstructor), so adding it also makes that path read the struct instead of returning an
unpopulated instance.

Together this restores sizeof(kstat_ctl_t) to 24 and removes both the overrunning read and the overrunning write.

This changes the type of a public field, so it is source- and binary-incompatible for anyone reading kc_chain today — though reading it has never returned anything meaningful, since the bytes it decodes are whatever follows the allocation. Callers that only pass KstatCtl to the library functions, which I expect is nearly all of them, are unaffected.

I've worked around it downstream by re-declaring the four kstat_ctl_t *-taking entry points with that argument as an opaque Pointer, which also works and needs no JNA change, but the mapping should be correct for everyone.

Unrelated but adjacent

Kstat.ks_update and Kstat.ks_snapshot are declared int where C has function pointers. On LP64 this is harmless by coincidence — a 4-byte field plus 4 bytes of alignment padding occupies the same space as the pointer, so every subsequent offset and the 184-byte total are unchanged, and Structure.write() does not touch padding, so the upper half of each pointer survives a round trip. Changing them to Pointer would be offset-neutral and clearer, if you want it in the same commit. Kstat is otherwise correct at its real 184 bytes.

Environment: JNA 5.19.1; OpenIndiana Hipster (illumos), x86-64; also applies to Oracle Solaris and to SPARC

Backwards compatibility

This could cause compile failures if we alter it in-place; so we may want to deprecate the broken version and create a new version with a slightly different name. Open to ideas or preferred renaming pattern.

Happy to submit this fix (since I submitted the broken version to start with).