Skip to content

Commit 3a0d724

Browse files
jakebaileydeadprogram
authored andcommitted
interp: fix partial store aliasing with previously loaded values
The optimization to skip cloning the destination object on partial stores when the buffer is already owned by the current memory view mutated obj.buffer.buf in place. The previous v.buf clone only handled the case where the store value itself aliased that buffer; it did not cover the more common case where an earlier partial load had returned a slice into the same buffer. The in-place store would then corrupt the previously loaded value, breaking code such as cloudflare/bn256 where the gfP arithmetic loads, computes, and stores within the same global during package initialization. Fix this in load instead: when the source object is owned by the current memory view, copy the loaded slice so the returned value is independent of the live buffer. The v.buf clone in store is no longer needed and is removed. Updates the regression test added in the previous commit to reflect the corrected behavior.
1 parent c980e48 commit 3a0d724

2 files changed

Lines changed: 8 additions & 8 deletions

File tree

interp/memory.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -316,10 +316,14 @@ func (mv *memoryView) load(p pointerValue, size uint32) value {
316316
panic("interp: load out of bounds")
317317
}
318318
v := obj.buffer.asRawValue(mv.r)
319-
loadedValue := rawValue{
320-
buf: v.buf[p.offset() : p.offset()+size],
319+
loadedBuf := v.buf[p.offset() : p.offset()+size]
320+
if _, writable := mv.objects[p.index()]; writable {
321+
// This object's buffer is owned by this view, which means a later
322+
// store may mutate it in place (see store below). Copy the loaded
323+
// slice so the returned value is not aliased with the live buffer.
324+
loadedBuf = append([]uint64(nil), loadedBuf...)
321325
}
322-
return loadedValue
326+
return rawValue{buf: loadedBuf}
323327
}
324328

325329
// Store to the value behind the given pointer. This overwrites the value in the
@@ -356,10 +360,6 @@ func (mv *memoryView) store(v value, p pointerValue) bool {
356360
buffer := obj.buffer.asRawValue(mv.r)
357361
obj.buffer = buffer
358362
v := v.asRawValue(mv.r)
359-
if writable {
360-
// A partial load from this object may share the destination buffer.
361-
v.buf = append([]uint64(nil), v.buf...)
362-
}
363363
for i := uint32(0); i < valueLen; i++ {
364364
buffer.buf[p.offset()+i] = v.buf[i]
365365
}

interp/testdata/store.out.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ target triple = "x86_64--linux"
55
@alias.src = local_unnamed_addr global [4 x i8] c"\05\06\07\08"
66
@alias.dst = local_unnamed_addr global [2 x i8] c"\09\07"
77
@reload.buf = local_unnamed_addr global [4 x i8] c"c\02\03\09"
8-
@reload.out = local_unnamed_addr global [2 x i8] c"c\02"
8+
@reload.out = local_unnamed_addr global [2 x i8] c"\01\02"
99

1010
define void @runtime.initAll() unnamed_addr {
1111
entry:

0 commit comments

Comments
 (0)