awcullen / opcua

OPC Unified Architecture (OPC UA) in Go.
MIT License
81 stars 18 forks source link

fatal error: checkptr: pointer arithmetic result points to invalid allocation #31

Closed elgohr closed 3 months ago

elgohr commented 7 months ago

When running go test -race ./... the error fatal error: checkptr: pointer arithmetic result points to invalid allocation occurs. This is a typical error when unsafe.Pointer rules are not respected (https://pkg.go.dev/unsafe#Pointer).

Running go vet ./... points into the direction of ua/binary_decoder.go:356:27: method ReadByte(value *byte) error should have signature ReadByte() (byte, error)

rjboer commented 7 months ago

Hi, the current implementation of ReadByte takes a pointer to a byte as an argument and modifies it, which differs from the standard signature.

// ReadByte reads a byte.
func (dec *BinaryDecoder) ReadByte(value *byte) error {
    if _, err := io.ReadFull(dec.r, dec.bs[:1]); err != nil {
        return BadDecodingError
    }
    *value = dec.bs[0]
    return nil
}

Then the fix would be something along these lines?

// ReadByte reads a byte.
func (dec *BinaryDecoder) ReadByte() (byte, error) {
    var b [1]byte
    if _, err := io.ReadFull(dec.r, b[:]); err != nil {
        return 0, err // Return zero value for byte and the error
    }
    return b[0], nil // Return the byte read and nil for error
}

@awcullen, what is your view on this? and did you transpile this from open62541 or...?

awcullen commented 4 months ago

@elgohr Thanks so much for reporting this issue. When encoding/decoding a slice, the library calculated one address too many. Now all tests pass.

I would prefer to leave ReadByte and the other decoder signatures unchanged. I would rather not allocate new return values.