This is an automated email from the ASF dual-hosted git repository.
chaokunyang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/fory.git
The following commit(s) were added to refs/heads/main by this push:
new b20311fe1 fix(go): ensure physical buffer space for unsafe varint
fast-paths (#3613)
b20311fe1 is described below
commit b20311fe15839e39b6b21ac28587424a62d3e3ab
Author: Ayush Kumar <[email protected]>
AuthorDate: Fri Apr 24 17:43:59 2026 +0530
fix(go): ensure physical buffer space for unsafe varint fast-paths (#3613)
## Why?
The Go runtime's struct serialization fast-path in `struct.go` violates
the documented contract of `UnsafePutVarUint32` and
`UnsafeReadVarUint32` in `buffer.go`.
- `UnsafePutVarUint32` documents that the caller must call `Reserve(8)`
(because it performs an 8-byte bulk write for 5-byte varints), but
`struct.go` only calls `Reserve(MaxVarintSize)`, which is 5 for
uint32/int32 varint fields.
- `UnsafeReadVarUint32` physically reads 8 bytes, but the fast-path
guard in `struct.go` only checks `remaining() >= MaxVarintSize` (which
can be 5).
## What does this PR do?
- Add +8 byte padding to struct varint reservation and remaining-check
guardrails in struct.go. This ensures that the underlying unsafe bulk
memory operations (8-byte loads/stores) always stay within the allocated
backing array, even when the logical varint size is smaller (e.g., 5
bytes).
- Includes a regression test in buffer_test.go verifying the physical
write width of `UnsafePutVarUint32`.
## Related issues
Closes #3612
## AI Contribution Checklist
- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence of the final clean AI review results
from both fresh reviewers on the current PR diff or current HEAD after
the latest code changes.
## Does this PR introduce any user-facing change?
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
## Benchmark
---
go/fory/buffer_test.go | 30 ++++++++++++++++++++++++++++++
go/fory/struct.go | 6 ++++--
go/fory/struct_test.go | 26 ++++++++++++++++++++++++++
3 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/go/fory/buffer_test.go b/go/fory/buffer_test.go
index c00ac1dde..a65d49a7a 100644
--- a/go/fory/buffer_test.go
+++ b/go/fory/buffer_test.go
@@ -81,3 +81,33 @@ func checkVarintWrite(t *testing.T, buf *ByteBuffer, value
int32) {
require.Equal(t, buf.ReaderIndex(), buf.WriterIndex())
require.Equal(t, value, varInt)
}
+
+// TestUnsafePutVarUint32PhysicalWriteWidth verifies that UnsafePutVarUint32
performs
+// an 8-byte physical write for 5-byte varints and that Reserve(8) (as
required by
+// the contract) keeps those 8 bytes within the backing array.
+func TestUnsafePutVarUint32PhysicalWriteWidth(t *testing.T) {
+ const sentinelByte = byte(0xAB)
+ const totalCap = 16
+ backing := make([]byte, totalCap, totalCap)
+
+ // Fill [8, totalCap) with sentinels; [0, 8) is the reserved window.
+ for i := 8; i < totalCap; i++ {
+ backing[i] = sentinelByte
+ }
+
+ // Expose 8 bytes of len, matching Reserve(8) contract.
+ buf := NewByteBuffer(backing[:8])
+
+ // Reserve(8) should return immediately as len(data) is already 8.
+ buf.Reserve(8)
+
+ // Encode value >= 2^28 (5 varint bytes) which triggers 8-byte bulk
write.
+ written := buf.UnsafePutVarUint32(0, 1<<28)
+ require.Equal(t, 5, written, "expected 5 logical bytes written")
+
+ // Verify bytes [8, totalCap) remain untouched by the 8-byte bulk write.
+ for i := 8; i < totalCap; i++ {
+ require.Equal(t, sentinelByte, backing[i],
+ "byte at index %d is outside the 8-byte reserved window
and must not be written", i)
+ }
+}
diff --git a/go/fory/struct.go b/go/fory/struct.go
index 52e53ad99..6f77aeedb 100644
--- a/go/fory/struct.go
+++ b/go/fory/struct.go
@@ -339,7 +339,8 @@ func (s *structSerializer) WriteData(ctx *WriteContext,
value reflect.Value) {
// - Reserve max size once, track offset locally, update writerIndex
once at end
//
==========================================================================
if s.fieldGroup.MaxVarintSize > 0 {
- buf.Reserve(s.fieldGroup.MaxVarintSize)
+ // +8 padding for UnsafePutVarUint32 bulk write (8 bytes
physically written for 5-byte varints)
+ buf.Reserve(s.fieldGroup.MaxVarintSize + 8)
offset := buf.WriterIndex()
for _, field := range s.fieldGroup.PrimitiveVarintFields {
@@ -1530,7 +1531,8 @@ func (s *structSerializer) ReadData(ctx *ReadContext,
value reflect.Value) {
// Note: For tagged int64/uint64, we can't use unsafe reads because
they need bounds checking
if len(s.fieldGroup.PrimitiveVarintFields) > 0 {
err := ctx.Err()
- if buf.remaining() >= s.fieldGroup.MaxVarintSize {
+ // +8 padding for readVarUint32Fast bulk load (8 bytes
physically read regardless of varint length)
+ if buf.remaining() >= s.fieldGroup.MaxVarintSize+8 {
for _, field := range
s.fieldGroup.PrimitiveVarintFields {
fieldPtr := unsafe.Add(ptr, field.Offset)
optInfo := optionalInfo{}
diff --git a/go/fory/struct_test.go b/go/fory/struct_test.go
index d4d42c130..c2d2041b7 100644
--- a/go/fory/struct_test.go
+++ b/go/fory/struct_test.go
@@ -617,3 +617,29 @@ func TestFloat16StructField(t *testing.T) {
// Specific value check
require.Equal(t, float32(1.5), res.F16.Float32())
}
+
+// TestVarintFastPathTightBuffer exercises the varint fast-path with a single
uint32 field.
+// Serializing a value requiring 5 varint bytes exercises the 8-byte bulk
write.
+// Deserializing from a tight buffer (len==cap) exercises the read guard,
ensuring
+// the 8-byte bulk load does not read past the end of the backing array.
+func TestVarintFastPathTightBuffer(t *testing.T) {
+ type SingleVarintStruct struct {
+ // compress=true forces the varint fast path.
+ Value uint32 `fory:"compress=true"`
+ }
+
+ f := New(WithXlang(false))
+ require.NoError(t, f.RegisterStruct(SingleVarintStruct{}, 7001))
+
+ // 1<<28 requires 5 varint bytes, forcing the 8-byte bulk write path.
+ obj := SingleVarintStruct{Value: 1 << 28}
+
+ data, err := f.Serialize(&obj)
+ require.NoError(t, err)
+
+ // Deserialize from a tight buffer where len == cap. This ensures the
read guard
+ // properly handles bulk loads that would otherwise overrun the slice.
+ var out SingleVarintStruct
+ require.NoError(t, f.Deserialize(data, &out))
+ require.Equal(t, obj.Value, out.Value)
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
