WIP: SecpPrivKeySegment impl for FFM - #482
Conversation
8debe20 to
05b8881
Compare
|
Commits 1-4 can be rebased and squashed after PR #483 is merged. |
Add SecpPrivKeySegment, an implementation of SecpPrivKey that keeps the data in an owned Auto memory segment. This segment will be disposed by Java GC after the SecpPrivKeySegment wrapper class is unreachable. The segment() method will return a read-only segment that can be directly passed to secp256k1 foreign functions. In the FFM implementation of ecPrivKeyCreate() create and return a SecpPrivKeySegment rather than the generic SecpPrivKeyImpl. The privKeySeg() method will return the read-only, pre-existing segment if a SecpPrivKeySegment is passed or create a temporary, writable segment if another SecpPrivKey is passed. zeroIfTemp() will zero the temporary segments.
This is an experimental, partial implementation backed by a MemorySegment in internal libsecp256k1 format. It's not clear to me yet what the most efficient format to use is, should it be the internal format, serialized uncompressed (65 bytes), or as two 32 byte fields. I suppose it depends upon which conversions are most common. For this implementation .segment() returns a ready-to-use segment for most native pubKey operations, but conversions are required for the other getters.
05b8881 to
98b61de
Compare
|
This should be rebased after PR #506 is merged. |
liamgilligan
left a comment
liamgilligan
left a comment
There was a problem hiding this comment.
Looks good. I assume that we are relying on the secp instance to validate the priv/pubkey segments used by the constructors -- is this a good decision?
| return "Secp256k1/" + ProviderId.LIBSECP256K1_FFM; | ||
| } | ||
|
|
||
| /// Fill temporary segments with zeros. In this context, non-temporary segments are read-only |
There was a problem hiding this comment.
I think this comment can be a little more clear. Perhaps it should be stated that generally non-temporary segments are from the Secp*Segment objects and temporary segments are all others.
There was a problem hiding this comment.
Maybe the method should be named zeroIfWritable? It's a low-level helper method and it doesn't really know what is temporary or not -- it just zeros MemorySegments that are writable. We could then document how it used elsewhere.
|
|
||
| @Override | ||
| public void destroy() { | ||
| // TODO: Make sure the zeroing is not optimized out by the compiler or JIT |
There was a problem hiding this comment.
Here is the C equivalent.
I believe this may work:
private static final VarHandle BYTE_VH = ValueLayout.JAVA_BYTE.varHandle();
public void destroy() {
if (destroyed) return;
for (long i = 0, n = segment.byteSize(); i < n; i++) {
BYTE_VH.setVolatile(segment, i, (byte) 0);
}
destroyed = true;
}This basically allows for volatile writes to off-heap memory, which shouldn't be optimized away.
There was a problem hiding this comment.
That's in interesting idea. I think we should find out whether the JDK/FFM can optimize out a call to fill() and if we need to do what you are suggesting, we'll do it.
At the very least should document in the constructors that we are assuming valid data. Since these are internal classes I think it is ok to make (and document) that assumption if we can be sure it always holds. |
2 participants
No description provided.