Robert Griesemer [Fri, 11 Apr 2014 04:46:00 +0000 (21:46 -0700)]
bufio: fix potential endless loop in ReadByte
Also: Simplify ReadSlice implementation and
ensure that it doesn't call fill() with a full
buffer (this caused a failure in net/textproto
TestLargeReadMIMEHeader because fill() wasn't able
to read more data).
There is a race condition that causes spurious wakeup from Wait
in the following case:
G1: decrement wg.counter, observe the counter is now 0
(should unblock goroutines queued *at this moment*)
G2: increment wg.counter
G2: call Wait() to add itself to the wait queue
G1: acquire wg.m, unblock all waiting goroutines
In the last step G2 is spuriously woken up by G1.
Fixes #7734.
net/http: don't reuse Transport connection unless Request.Write finished
In a typical HTTP request, the client writes the request, and
then the server replies. Go's HTTP client code (Transport) has
two goroutines per connection: one writing, and one reading. A
third goroutine (the one initiating the HTTP request)
coordinates with those two.
Because most HTTP requests are done when the server replies,
the Go code has always handled connection reuse purely in the
readLoop goroutine.
But if a client is writing a large request and the server
replies before it's consumed the entire request (e.g. it
replied with a 403 Forbidden and had no use for the body), it
was possible for Go to re-select that connection for a
subsequent request before we were done writing the first. That
wasn't actually a data race; the second HTTP request would
just get enqueued to write its request on the writeLoop. But
because the previous writeLoop didn't finish writing (and
might not ever), that connection is in a weird state. We
really just don't want to get into a state where we're
re-using a connection when the server spoke out of turn.
This CL changes the readLoop goroutine to verify that the
writeLoop finished before returning the connection.
In the process, it also fixes a potential goroutine leak where
a connection could close but the recycling logic could be
blocked forever waiting for the client to read to EOF or
error. Now it also selects on the persistConn's close channel,
and the closer of that is no longer the readLoop (which was
dead locking in some cases before). It's now closed at the
same place the underlying net.Conn is closed. This likely fixes
or helps Issue 7620.
Also addressed some small cosmetic things in the process.
David du Colombier [Thu, 10 Apr 2014 04:37:30 +0000 (06:37 +0200)]
runtime: no longer skip stack growth test in short mode
We originally decided to skip this test in short mode
to prevent the parallel runtime test to timeout on the
Plan 9 builder. This should no longer be required since
the issue was fixed in CL 86210043.
David du Colombier [Thu, 10 Apr 2014 04:36:20 +0000 (06:36 +0200)]
runtime: fix semasleep on Plan 9
If you pass ns = 100,000 to this function, timediv will
return ms = 0. tsemacquire in /sys/src/9/port/sysproc.c
will return immediately when ms == 0 and the semaphore
cannot be acquired immediately - it doesn't sleep - so
notetsleep will spin, chewing cpu and repeatedly reading
the time, until the 100us have passed.
Thanks to the time reads it won't take too many iterations,
but whatever we are waiting for does not get a chance to
run. Eventually the notetsleep spin loop returns and we
end up in the stoptheworld spin loop - actually a sleep
loop but we're not doing a good job of sleeping.
After 100ms or so of this, the kernel says enough and
schedules a different thread. That thread manages to do
whatever we're waiting for, and the spinning in the other
thread stops. If tsemacquire had actually slept, this
would have happened much quicker.
go-mode on Emacs 23 wrongly recognizes a backquote in a comment or
a string as a start of a raw string literal. Below is an example
that go-mode does not work well. This patch is to fix that issue.
Rob Pike [Wed, 9 Apr 2014 05:57:50 +0000 (15:57 +1000)]
html/template: fix two unrelated bugs
1) The code to catch an exception marked the template as escaped
when it was not yet, which caused subsequent executions of the
template to not escape properly.
2) ensurePipelineContains needs to handled Field as well as
Identifier nodes.
David du Colombier [Wed, 9 Apr 2014 04:41:14 +0000 (06:41 +0200)]
runtime: fix GOTRACEBACK on Plan 9
Getenv() should not call malloc when called from
gotraceback(). Instead, we return a static buffer
in this case, with enough room to hold the longest
value.
On Plan 9 gotraceback calls getenv calls malloc, and we gotraceback
on every call to gentraceback, which happens during garbage collection.
Honestly I don't even know how this works on Plan 9.
I suspect it does not, and that we are getting by because
no one has tried to run with $GOTRACEBACK set at all.
This will speed up all the other systems by epsilon, since they
won't call getenv and atoi repeatedly.
Adam Langley [Tue, 8 Apr 2014 23:32:48 +0000 (16:32 -0700)]
crypto/(ec)dsa: use Fermat's inversion.
Now that we have a constant-time P-256 implementation, it's worth
paying more attention elsewhere.
The inversion of k in (EC)DSA was using Euclid's algorithm which isn't
constant-time. This change switches to Fermat's algorithm, which is
much better. However, it's important to note that math/big itself isn't
constant time and is using a 4-bit window for exponentiation with
variable memory access patterns.
(Since math/big depends quite deeply on its values being in minimal (as
opposed to fixed-length) represetation, perhaps crypto/elliptic should
grow a constant-time implementation of exponentiation in the scalar
field.)
reflect, runtime: fix crash in GC due to reflect.call + precise GC
Given
type Outer struct {
*Inner
...
}
the compiler generates the implementation of (*Outer).M dispatching to
the embedded Inner. The implementation is logically:
func (p *Outer) M() {
(p.Inner).M()
}
but since the only change here is the replacement of one pointer
receiver with another, the actual generated code overwrites the
original receiver with the p.Inner pointer and then jumps to the M
method expecting the *Inner receiver.
During reflect.Value.Call, we create an argument frame and the
associated data structures to describe it to the garbage collector,
populate the frame, call reflect.call to run a function call using
that frame, and then copy the results back out of the frame. The
reflect.call function does a memmove of the frame structure onto the
stack (to set up the inputs), runs the call, and the memmoves the
stack back to the frame structure (to preserve the outputs).
Originally reflect.call did not distinguish inputs from outputs: both
memmoves were for the full stack frame. However, in the case where the
called function was one of these wrappers, the rewritten receiver is
almost certainly a different type than the original receiver. This is
not a problem on the stack, where we use the program counter to
determine the type information and understand that during (*Outer).M
the receiver is an *Outer while during (*Inner).M the receiver in the
same memory word is now an *Inner. But in the statically typed
argument frame created by reflect, the receiver is always an *Outer.
Copying the modified receiver pointer off the stack into the frame
will store an *Inner there, and then if a garbage collection happens
to scan that argument frame before it is discarded, it will scan the
*Inner memory as if it were an *Outer. If the two have different
memory layouts, the collection will intepret the memory incorrectly.
runtime/race: more precise handling of channel synchronization
It turns out there is a relatively common pattern that relies on
inverted channel semaphore:
gate := make(chan bool, N)
for ... {
// limit concurrency
gate <- true
go func() {
foo(...)
<-gate
}()
}
// join all goroutines
for i := 0; i < N; i++ {
gate <- true
}
So handle synchronization on inverted semaphores with cap>1.
Fixes #7718.
Keith Randall [Tue, 8 Apr 2014 00:35:44 +0000 (17:35 -0700)]
runtime: fix heapdump bugs.
Iterate the right number of times in arrays and channels.
Handle channels with zero-sized objects in them.
Output longer type names if we have them.
Compute argument offset correctly.
Lucio De Re [Mon, 7 Apr 2014 15:40:13 +0000 (08:40 -0700)]
libbio, libmach: warnings from the Plan 9 tool chain
Superficial inconsistencies that trigger warnings in
Plan 9. Small enough to be considered trivial and
seemingly benign outside of the Plan 9 environment.
net: fix data race in benchmark
If an error happens on a connection, server goroutine can call b.Logf
after benchmark finishes.
So join both client and server goroutines.
Update #7718
cmd/gc, runtime: make GODEBUG=gcdead=1 mode work with liveness
Trying to make GODEBUG=gcdead=1 work with liveness
and in particular ambiguously live variables.
1. In the liveness computation, mark all ambiguously live
variables as live for the entire function, except the entry.
They are zeroed directly after entry, and we need them not
to be poisoned thereafter.
2. In the liveness computation, compute liveness (and deadness)
for all parameters, not just pointer-containing parameters.
Otherwise gcdead poisons untracked scalar parameters and results.
3. Fix liveness debugging print for -live=2 to use correct bitmaps.
(Was not updated for compaction during compaction CL.)
4. Correct varkill during map literal initialization.
Was killing the map itself instead of the inserted value temp.
5. Disable aggressive varkill cleanup for call arguments if
the call appears in a defer or go statement.
6. In the garbage collector, avoid bug scanning empty
strings. An empty string is two zeros. The multiword
code only looked at the first zero and then interpreted
the next two bits in the bitmap as an ordinary word bitmap.
For a string the bits are 11 00, so if a live string was zero
length with a 0 base pointer, the poisoning code treated
the length as an ordinary word with code 00, meaning it
needed poisoning, turning the string into a poison-length
string with base pointer 0. By the same logic I believe that
a live nil slice (bits 11 01 00) will have its cap poisoned.
Always scan full multiword struct.
7. In the runtime, treat both poison words (PoisonGC and
PoisonStack) as invalid pointers that warrant crashes.
Manual testing as follows:
- Create a script called gcdead on your PATH containing:
#!/bin/bash
GODEBUG=gcdead=1 GOGC=10 GOTRACEBACK=2 exec "$@"
- Now you can build a test and then run 'gcdead ./foo.test'.
- More importantly, you can run 'go test -short -exec gcdead std'
to run all the tests.
Fixes #7676.
While here, enable the precise scanning of slices, since that was
disabled due to bugs like these. That now works, both with and
without gcdead.
runtime: test malformed address fault and fix on OS X
The garbage collector poison pointers
(0x6969696969696969 and 0x6868686868686868)
are malformed addresses on amd64.
That is, they are not 48-bit addresses sign extended
to 64 bits. This causes a different kind of hardware fault
than the usual 'unmapped page' when accessing such
an address, and OS X 10.9.2 sends the resulting SIGSEGV
incorrectly, making it look like it was user-generated
rather than kernel-generated and does not include the
faulting address. This means that in GODEBUG=gcdead=1
mode, if there is a bug and something tries to dereference
a poisoned pointer, the runtime delivers the SIGSEGV to
os/signal and returns to the faulting code, which faults
again, causing the process to hang instead of crashing.
Fix by rewriting "user-generated" SIGSEGV on OS X to
look like a kernel-generated SIGSEGV with fault address
0xb01dfacedebac1e.
I chose that address because (1) when printed in hex
during a crash, it is obviously spelling out English text,
(2) there are no current Google hits for that pointer,
which will make its origin easy to find once this CL
is indexed, and (3) it is not an altogether inaccurate
description of the situation.
Add a test. Maybe other systems will break too.
LGTM=khr
R=golang-codereviews, khr
CC=golang-codereviews, iant, ken
https://golang.org/cl/83270049
runtime: handle fault during runtime more like unexpected fault address
Delaying the runtime.throw until here will print more information.
In particular it will print the signal and code values, which means
it will show the fault address.
The canpanic checks were added recently, in CL 75320043.
They were just not added in exactly the right place.
cmd/gc, runtime: optimize map[string] lookup from []byte key
Brad has been asking for this for a while.
I have resisted because I wanted to find a more general way to
do this, one that would keep the performance of code introducing
variables the same as the performance of code that did not.
(See golang.org/issue/3512#c20).
I have not found the more general way, and recent changes to
remove ambiguously live temporaries have blown away the
property I was trying to preserve, so that's no longer a reason
not to make the change.
Fixes #3512.
LGTM=iant
R=iant
CC=bradfitz, golang-codereviews, khr, r
https://golang.org/cl/83740044
This is just testing the status quo, so that any future attempt
to change it will make the test break and redirect the person
making the change to look at issue 6027.
runtime: fix fault during arm software floating point
The software floating point runs with m->locks++
to avoid being preempted; recognize this case in panic
and undo it so that m->locks is maintained correctly
when panicking.
The old limit of 5 was chosen because we didn't actually know how
many bytes of arguments there were; 5 was a halfway point between
printing some useful information and looking ridiculous.
Now we know how many bytes of arguments there are, and we stop
the printing when we reach that point, so the "looking ridiculous" case
doesn't happen anymore: we only print actual argument words.
The cutoff now serves only to truncate very long (but real) argument lists.
In multiple debugging sessions recently (completely unrelated bugs)
I have been frustrated by not seeing more of the long argument lists:
5 words is only 2.5 interface values or strings, and not even 2 slices.
Double the max amount we'll show.
LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews, iant, r
https://golang.org/cl/83850043
Keith Randall [Thu, 3 Apr 2014 02:46:47 +0000 (19:46 -0700)]
cmd/ld: get rid of map.bucket's data field from dwarf info.
The data field is the generic array that acts as a standin
for the keys and values arrays for the generic runtime code.
We want to substitute the keys and values arrays for the data
array, not just add keys and values in addition to it.