Skip to content

chore: Add safety note around zero initializing JavaOutputBuf#265

Merged
garyschulte merged 3 commits intobesu-eth:mainfrom
kevaundray:kw/add-safety-warning-around-zero-init
Mar 24, 2025
Merged

chore: Add safety note around zero initializing JavaOutputBuf#265
garyschulte merged 3 commits intobesu-eth:mainfrom
kevaundray:kw/add-safety-warning-around-zero-init

Conversation

@kevaundray
Copy link
Copy Markdown
Contributor

No description provided.

@kevaundray
Copy link
Copy Markdown
Contributor Author

In nonMontgomeryMarshal. we have:

func nonMontgomeryMarshal(xVal, yVal *fp.Element, output *C.char, outputOffset int) error {
	// Convert g1.X and g1.Y to big.Int using the BigInt method
	var x big.Int
	xVal.BigInt(&x)
	xBytes := x.Bytes()
	xLen := len(xBytes)

	if xLen > 0 {
		// Copy x to output at offset (64 - xLen)
		C.memcpy(unsafe.Pointer(uintptr(unsafe.Pointer(output))+uintptr(outputOffset+64-xLen)), unsafe.Pointer(&xBytes[0]), C.size_t(xLen))
	}

	var y big.Int
	yVal.BigInt(&y)
	yBytes := y.Bytes()
	yLen := len(yBytes)

	if yLen > 0 {
		// Copy y to output at offset (128 - yLen)
		C.memcpy(unsafe.Pointer(uintptr(unsafe.Pointer(output))+uintptr(outputOffset+128-yLen)), unsafe.Pointer(&yBytes[0]), C.size_t(yLen))
	}
	return nil
}

If xVal or yVal is zero, then len(xBytes) or len(yBytes) will be zero, which means that the buffer is not written to. This is only safe if the buffer is zero initialized.

@kevaundray kevaundray marked this pull request as ready for review March 19, 2025 21:25
@kevaundray
Copy link
Copy Markdown
Contributor Author

Maybe the java code should zero out the output buffer/ensure it is zeroed out before passing it to the golang code

@kevaundray kevaundray force-pushed the kw/add-safety-warning-around-zero-init branch 2 times, most recently from 946c023 to c67cb74 Compare March 19, 2025 22:51
Signed-off-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>
@kevaundray kevaundray force-pushed the kw/add-safety-warning-around-zero-init branch from c67cb74 to c3e0f64 Compare March 19, 2025 22:52
@garyschulte
Copy link
Copy Markdown
Contributor

garyschulte commented Mar 19, 2025

Maybe the java code should zero out the output buffer/ensure it is zeroed out before passing it to the golang code

In AbstractBLS12PrecompiledContract, Besu sends a newly initialized byte array for the output: https://github.com/hyperledger/besu/blob/main/evm/src/main/java/org/hyperledger/besu/evm/precompile/AbstractBLS12PrecompiledContract.java#L111

The default value for a byte is 0 in java, so the byte array is always zero initialized. I definitely see how this would be a problem if a non-zero initialized array were passed. Besu's usage is safe, but it is worth noting the zero-initialized requirement.

IMO your added comments are a sufficient call-out for safety here.

edit: perhaps we should add that to LibGnarkEIP2537 javadoc as well

@garyschulte garyschulte merged commit 0d7e1f0 into besu-eth:main Mar 24, 2025
12 checks passed
@garyschulte garyschulte mentioned this pull request Jun 9, 2025
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