New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal/bytealg: valgrind reports invalid reads by C.GoString #27610
Comments
Thanks for filing an issue. cc @TocarIP |
There is code in The place where the out-of-bounds read is generated is src/runtime/string.go:findnull. We do this:
We're essentially looking for a null one page at a time. We're passing a range to IndexByte that can range outside a small allocation. It's safe with respect to the virtual memory because we don't stray off the page, but I can certainly see how it would confuse Valgrind. |
Indeed; it's safe inasmuch as it won't run off a page, but it is reading uninitialised memory (in that that part of the page has not been 'allocated' to userspace by a That said, the aforementioned pthread "possibly lost" allocations are also Valgrind noise. Maybe we could document suppressions/include a sample suppressions file? I suspect there are many more I've yet to hit, though. |
Yes, it would be nice if we had a clean Valgrind run, or had a way to induce one with a config file or something. We do this over-reading in a few places in the Go runtime. Normally the over-reading happens in the Go heap, which Valgrind presumably doesn't care about. I think How does C handle this? I would think C library functions like |
Yeah, that makes a lot of sense. I haven't looked into it yet, but I do assume the C library does this as well, and that Valgrind has its own |
I think all of the cases in the Go stdlib would be handled by the Valgrind option |
I was on 3.12.0 (Oct 2016); have just tried on the latest stable, 3.13.0, and the result is the same: $ valgrind --partial-loads-ok=yes ./sscce
==15735== Memcheck, a memory error detector
==15735== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==15735== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==15735== Command: ./sscce
==15735==
==15735== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==15735== the SIGRT32 signal is used internally by Valgrind
==15735== Warning: ignored attempt to set SIGRT32 handler in sigaction();
==15735== the SIGRT32 signal is used internally by Valgrind
==15735== Warning: client switching stacks? SP change: 0x1fff0001f0 --> 0xc0000347d8
==15735== to suppress, use: --max-stackframe=687211759080 or greater
==15735== Warning: client switching stacks? SP change: 0xc000034790 --> 0x1fff0002a0
==15735== to suppress, use: --max-stackframe=687211758832 or greater
==15735== Warning: client switching stacks? SP change: 0x1fff0002a0 --> 0xc000034790
==15735== to suppress, use: --max-stackframe=687211758832 or greater
==15735== further instances of this message will not be shown.
==15735== Conditional jump or move depends on uninitialised value(s)
==15735== at 0x40265B: indexbytebody (/usr/local/go/src/internal/bytealg/indexbyte_amd64.s:151)
==15735==
==15735==
==15735== HEAP SUMMARY:
==15735== in use at exit: 1,176 bytes in 5 blocks
==15735== total heap usage: 10 allocs, 5 frees, 1,316 bytes allocated
==15735==
==15735== LEAK SUMMARY:
==15735== definitely lost: 0 bytes in 0 blocks
==15735== indirectly lost: 0 bytes in 0 blocks
==15735== possibly lost: 1,152 bytes in 4 blocks
==15735== still reachable: 24 bytes in 1 blocks
==15735== suppressed: 0 bytes in 0 blocks
==15735== Rerun with --leak-check=full to see details of leaked memory
==15735==
==15735== For counts of detected and suppressed errors, rerun with: -v
==15735== Use --track-origins=yes to see where uninitialised values come from
==15735== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) |
It doesn't seem like we should change how the Go library behaves only to make valgrind happy. We need some way to tell valgrind that this particular read of uninitialized memory is OK. Is there any way to do that? |
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
)?What did you do?
(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
:I understand some work has been done to ensure
IndexByte
doesn't run over a page boundary (#24206), and so that this is unlikely to have a negative effect. Should I just add a suppression and call it a day?The text was updated successfully, but these errors were encountered: