internal/bytealg: valgrind reports invalid reads by C.GoString

What version of Go are you using (go version)?

go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/kivikakk/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/kivikakk/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build515123667=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

/*
#include <string.h>
#include <stdlib.h>
char* s() {
        return strdup("hello");
}
*/
import "C"
import "unsafe"

func main() {
        s := C.s()
        C.GoString(s)
        C.free(unsafe.Pointer(s))
}

$ go build
$ valgrind ./sscce
==11241== Memcheck, a memory error detector
==11241== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==11241== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==11241== Command: ./sscce
==11241==
==11241== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==11241==          the SIGRT32 signal is used internally by Valgrind
==11241== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==11241==          the SIGRT32 signal is used internally by Valgrind
==11241== Warning: client switching stacks?  SP change: 0xfff0001b0 --> 0xc0000367d8
==11241==          to suppress, use: --max-stackframe=755931244072 or greater
==11241== Warning: client switching stacks?  SP change: 0xc000036790 --> 0xfff000260
==11241==          to suppress, use: --max-stackframe=755931243824 or greater
==11241== Warning: client switching stacks?  SP change: 0xfff000260 --> 0xc000036790
==11241==          to suppress, use: --max-stackframe=755931243824 or greater
==11241==          further instances of this message will not be shown.
==11241== Conditional jump or move depends on uninitialised value(s)
==11241==    at 0x40265B: indexbytebody (/usr/local/go/src/internal/bytealg/indexbyte_amd64.s:151)
==11241==
==11241==
==11241== HEAP SUMMARY:
==11241==     in use at exit: 1,200 bytes in 6 blocks
==11241==   total heap usage: 10 allocs, 4 frees, 1,310 bytes allocated
==11241==
==11241== LEAK SUMMARY:
==11241==    definitely lost: 0 bytes in 0 blocks
==11241==    indirectly lost: 0 bytes in 0 blocks
==11241==      possibly lost: 1,152 bytes in 4 blocks
==11241==    still reachable: 48 bytes in 2 blocks
==11241==         suppressed: 0 bytes in 0 blocks
==11241== Rerun with --leak-check=full to see details of leaked memory
==11241==
==11241== For counts of detected and suppressed errors, rerun with: -v
==11241== Use --track-origins=yes to see where uninitialised values come from
==11241== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

(Ignore the "possibly lost" blocks; they're pthreads started by the Go runtime.)

What did you expect to see?

No conditional jump/move depending on uninitialised values.

What did you see instead?

A conditional jump/move depending on uninitialised values.

---

The nature of the issue becomes more apparent if you run Valgrind with --partial-loads-ok=no:

$ valgrind --partial-loads-ok=no ./sscce
==11376== Memcheck, a memory error detector
==11376== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==11376== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==11376== Command: ./sscce
==11376==
==11376== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==11376==          the SIGRT32 signal is used internally by Valgrind
==11376== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==11376==          the SIGRT32 signal is used internally by Valgrind
==11376== Warning: client switching stacks?  SP change: 0xfff0001b0 --> 0xc0000367d8
==11376==          to suppress, use: --max-stackframe=755931244072 or greater
==11376== Warning: client switching stacks?  SP change: 0xc000036790 --> 0xfff000260
==11376==          to suppress, use: --max-stackframe=755931243824 or greater
==11376== Warning: client switching stacks?  SP change: 0xfff000260 --> 0xc000036790
==11376==          to suppress, use: --max-stackframe=755931243824 or greater
==11376==          further instances of this message will not be shown.
==11376== Invalid read of size 32
==11376==    at 0x40264E: indexbytebody (/usr/local/go/src/internal/bytealg/indexbyte_amd64.s:148)
==11376==  Address 0x53f47c0 is 0 bytes inside a block of size 12 alloc'd
==11376==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==11376==    by 0x45165D: s (main.go:7)
==11376==    by 0x4516A5: _cgo_a004886745c9_Cfunc_s (cgo-gcc-prolog:54)
==11376==    by 0x44A0DF: runtime.asmcgocall (/usr/local/go/src/runtime/asm_amd64.s:637)
==11376==    by 0x7: ???
==11376==    by 0x6C287F: ??? (in /home/kivikakk/sscce/sscce)
==11376==    by 0xFFF00024F: ???
==11376==    by 0x4462B1: runtime.(*mcache).nextFree.func1 (/usr/local/go/src/runtime/malloc.go:749)
==11376==    by 0x448905: runtime.systemstack (/usr/local/go/src/runtime/asm_amd64.s:351)
==11376==    by 0x4283BF: ??? (/usr/local/go/src/runtime/proc.go:1146)
==11376==    by 0x448798: runtime.rt0_go (/usr/local/go/src/runtime/asm_amd64.s:201)
==11376==    by 0x451DEF: ??? (in /home/kivikakk/sscce/sscce)
==11376==
==11376==
==11376== HEAP SUMMARY:
==11376==     in use at exit: 1,200 bytes in 6 blocks
==11376==   total heap usage: 10 allocs, 4 frees, 1,316 bytes allocated
==11376==
==11376== LEAK SUMMARY:
==11376==    definitely lost: 0 bytes in 0 blocks
==11376==    indirectly lost: 0 bytes in 0 blocks
==11376==      possibly lost: 1,152 bytes in 4 blocks
==11376==    still reachable: 48 bytes in 2 blocks
==11376==         suppressed: 0 bytes in 0 blocks
==11376== Rerun with --leak-check=full to see details of leaked memory
==11376==
==11376== For counts of detected and suppressed errors, rerun with: -v
==11376== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

I understand some work has been done to ensure IndexByte doesn't run over a page boundary (https://github.com/golang/go/issues/24206), and so that this is unlikely to have a negative effect. Should I just add a suppression and call it a day?

{
   indexbytebody_loves_to_read
   Memcheck:Addr32
   fun:indexbytebody
}