Matthew Dempsky [Mon, 5 Aug 2019 20:12:01 +0000 (13:12 -0700)]
cmd/compile: fix "previous" position info for duplicate switch cases
Because the Node AST represents references to declared objects (e.g.,
variables, packages, types, constants) by directly pointing to the
referred object, we don't have use-position info for these objects.
For switch statements with duplicate cases, we report back where the
first duplicate value appeared. However, due to the AST
representation, if the value was a declared constant, we mistakenly
reported the constant declaration position as the previous case
position.
This CL reports back against the 'case' keyword's position instead, if
there's no more precise information available to us.
It also refactors code to emit the same "previous at" error message
for duplicate values in map literals.
Bryan C. Mills [Mon, 26 Aug 2019 20:44:36 +0000 (16:44 -0400)]
net/http: fix wantConnQueue memory leaks in Transport
I'm trying to keep the code changes minimal for backporting to Go 1.13,
so it is still possible for a handful of entries to leak,
but the leaks are now O(1) instead of O(N) in the steady state.
Longer-term, I think it would be a good idea to coalesce idleMu with
connsPerHostMu and clear entries out of both queues as soon as their
goroutines are done waiting.
The decoder called this function to check numbers being decoded into a
json.Number. However, these can't be quoted as strings, so the tokenizer
has already verified they are valid JSON numbers.
Verified this by adding a test with such an input. As expected, it
produces a syntax error, not the fmt.Errorf - that line could never
execute.
Since the only remaining non-test caller of isvalidnumber is in
encode.go, move the function there.
This change should slightly reduce the amount of work when decoding into
json.Number, though that isn't very common nor part of any current
benchmarks.
Daniel Martí [Sat, 10 Aug 2019 17:27:45 +0000 (19:27 +0200)]
cmd/compile: teach rulegen to remove unused decls
First, add cpu and memory profiling flags, as these are useful to see
where rulegen is spending its time. It now takes many seconds to run on
a recent laptop, so we have to keep an eye on what it's doing.
Second, stop writing '_ = var' lines to keep imports and variables used
at all times. Now that rulegen removes all such unused names, they're
unnecessary.
To perform the removal, lean on go/types to first detect what names are
unused. We can configure it to give us all the type-checking errors in a
file, so we can collect all "declared but not used" errors in a single
pass.
We then use astutil.Apply to remove the relevant nodes based on the line
information from each unused error. This allows us to apply the changes
without having to do extra parser+printer roundtrips to plaintext, which
are far too expensive.
We need to do multiple such passes, as removing an unused variable
declaration might then make another declaration unused. Two passes are
enough to clean every file at the moment, so add a limit of three passes
for now to avoid eating cpu uncontrollably by accident.
The resulting performance of the changes above is a ~30% loss across the
table, since go/types is fairly expensive. The numbers were obtained
with 'benchcmd Rulegen go run *.go', which involves compiling rulegen
itself, but that seems reflective of how the program is used.
name old time/op new time/op delta
Rulegen 5.61s ± 0% 7.36s ± 0% +31.17% (p=0.016 n=5+4)
name old user-time/op new user-time/op delta
Rulegen 7.20s ± 1% 9.92s ± 1% +37.76% (p=0.016 n=5+4)
name old sys-time/op new sys-time/op delta
Rulegen 135ms ±19% 169ms ±17% +25.66% (p=0.032 n=5+5)
name old peak-RSS-bytes new peak-RSS-bytes delta
Rulegen 71.0MB ± 2% 85.6MB ± 2% +20.56% (p=0.008 n=5+5)
We can live with a bit more resource usage, but the time/op getting
close to 10s isn't good. To win that back, introduce concurrency in
main.go. This further increases resource usage a bit, but the real time
on this quad-core laptop is greatly reduced. The final benchstat is as
follows:
name old time/op new time/op delta
Rulegen 5.61s ± 0% 3.97s ± 1% -29.26% (p=0.008 n=5+5)
name old user-time/op new user-time/op delta
Rulegen 7.20s ± 1% 13.91s ± 1% +93.09% (p=0.008 n=5+5)
name old sys-time/op new sys-time/op delta
Rulegen 135ms ±19% 269ms ± 9% +99.17% (p=0.008 n=5+5)
name old peak-RSS-bytes new peak-RSS-bytes delta
Rulegen 71.0MB ± 2% 226.3MB ± 1% +218.72% (p=0.008 n=5+5)
It might be possible to reduce the cpu or memory usage in the future,
such as configuring go/types to do less work, or taking shortcuts to
avoid having to run it many times. For now, ~2x cpu and ~4x memory usage
seems like a fair trade for a faster and better rulegen.
Finally, we can remove the old code that tried to remove some unused
variables in a hacky and unmaintainable way.
Change-Id: Iff9e83e3f253babf5a1bd48cc993033b8550cee6
Reviewed-on: https://go-review.googlesource.com/c/go/+/189798
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Andrew Gerrand [Fri, 31 May 2019 12:00:42 +0000 (22:00 +1000)]
archive/zip: remove unused special case
This removes a special case that was added to fix issue #10956, but that
was never actually effective. The code in the test case still fails to
read, so perhaps the zip64 support added in CL 6463050 inadvertently
caught this particular case.
It's possible that the original theorized bug still exists, but I'm not
convinced it was ever fixed.
Daniel Martí [Thu, 11 Jul 2019 12:27:19 +0000 (21:27 +0900)]
encoding/json: fix the broken "overwriting of data" tests
Because TestUnmarshal actually allocates a new value to decode into
using ptr's pointer type, any existing data is thrown away. This was
harmless in alomst all of the test cases, minus the "overwriting of
data" ones added in 2015 in CL 12209.
I spotted that nothing covered decoding a JSON array with few elements
into a slice which already had many elements. I initially assumed that
the code was buggy or that some code could be removed, when in fact
there simply wasn't any code covering the edge case.
Move those two tests to TestPrefilled, which already served a very
similar purpose. Remove the map case, as TestPrefilled already has
plenty of prefilled map cases. Moreover, we no longer reset an entire
map when decoding, as per the godoc:
To unmarshal a JSON object into a map, Unmarshal first
establishes a map to use. If the map is nil, Unmarshal allocates
a new map. Otherwise Unmarshal reuses the existing map, keeping
existing entries.
Finally, to ensure that ptr is used correctly in the future, make
TestUnmarshal error if it's anything other than a pointer to a zero
value. That is, the only correct use should be new(type). Don't rename
the ptr field, as that would be extremely noisy and cause unwanted merge
conflicts.
Daniel Martí [Thu, 16 May 2019 10:26:40 +0000 (11:26 +0100)]
cmd/compile: initial rulegen rewrite
rulegen.go produces plaintext Go code directly, which was fine for a
while. However, that's started being a bottleneck for making code
generation more complex, as we can only generate code directly one line
at a time.
Some workarounds were used, like multiple layers of buffers to generate
chunks of code, to then use strings.Contains to see whether variables
need to be defined or not. However, that's error-prone, verbose, and
difficult to work with.
A better approach is to generate an intermediate syntax tree in memory,
which we can inspect and modify easily. For example, we could run a
number of "passes" on the syntax tree before writing to disk, such as
removing unused variables, simplifying logic, or moving declarations
closer to their uses.
This is the first step in that direction, without changing any of the
generated code. We didn't use go/ast directly, as it's too complex for
our needs. In particular, we only need a few kinds of simple statements,
but we do want to support arbitrary expressions. As such, define a
simple set of statement structs, and add thin layers for printer.Fprint
and ast.Inspect.
A nice side effect of this change, besides removing some buffers and
string handling, is that we can now avoid passing so many parameters
around. And, while we add over a hundred lines of code, the tricky
pieces of code are now a bit simpler to follow.
While at it, apply some cleanups, such as replacing isVariable with
token.IsIdentifier, and consistently using log.Fatalf.
Follow-up CLs will start improving the generated code, also simplifying
the rulegen code itself. I've added some TODOs for the low-hanging fruit
that I intend to work on right after.
Updates #30810.
Change-Id: Ic371c192b29c85dfc4a001be7fbcbeec85facc9d
Reviewed-on: https://go-review.googlesource.com/c/go/+/177539
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Javier Revillas [Sun, 25 Aug 2019 16:05:41 +0000 (16:05 +0000)]
net/http: fix a typo in comments
HTTP is an initialism, not an acronym, where you pronounce each letter as a
word. It's "an H", not "a H".
Running `find src/net/http -type f | xargs grep -n 'an HTTP' | wc -l` shows
that the "an HTTP" form is used 67 times across the `net/http` package.
Furthermore, `find src/net/http -type f | xargs grep -n 'a HTTP' | wc -l`
yields only 4 results.
Change-Id: I219c292a9e2c9bf7a009dbfe82ea8b15874685e9
GitHub-Last-Rev: 6ebd095023af47444b6b0fc5b6d7b26d85f4c7b7
GitHub-Pull-Request: golang/go#33810
Reviewed-on: https://go-review.googlesource.com/c/go/+/191700 Reviewed-by: Toshihiro Shiino <shiino.toshihiro@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
cmd/compile: handle sign/zero extensions in prove, via update method
Array accesses with index types smaller than the machine word size may
involve a sign or zero extension of the index value before bounds
checking. Currently, this defeats prove because the facts about the
original index value don't flow through the sign/zero extension.
This CL fixes this by looking back through value-preserving sign/zero
extensions when adding facts via Update and, where appropriate, applying
the same facts using the pre-extension value. This fix is enhanced by
also looking back through value-preserving extensions within
ft.isNonNegative to infer whether the extended value is known to be
non-negative. Without this additional isNonNegative enhancement, this
logic is rendered significantly less effective by the limitation
discussed in the next paragraph.
In Update, the application of facts to pre-extension values is limited
to cases where the domain of the new fact is consistent with the type of
the pre-extension value. There may be cases where this cross-domain
passing of facts is valid, but distinguishing them from the invalid
cases is difficult for me to reason about and to implement.
Assessing which cases to allow requires details about the context and
inferences behind the fact being applied which are not available
within Update. Additional difficulty arises from the fact that the SSA
does not curently differentiate extensions added by the compiler for
indexing operations, extensions added by the compiler for implicit
conversions, or explicit extensions from the source.
Examples of some cases that would need to be filtered correctly for
cross-domain facts:
(1) A uint8 is zero-extended to int for indexing (a value-preserving
zeroExt). When, if ever, can signed domain facts learned about the int be
applied to the uint8?
(2) An int8 is sign-extended to int16 (value-preserving) for an equality
comparison. Equality comparison facts are currently always learned in both
the signed and unsigned domains. When, if ever, can the unsigned facts
learned about the int16, from the int16 != int16 comparison, be applied
to the original int8?
This is an alternative to CL 122695 and CL 174309. Compared to CL 122695,
this CL differs in that the facts added about the pre-extension value will
pass through the Update method, where additional inferences are processed
(e.g. fence-post implications, see #29964). CL 174309 is limited to bounds
checks, so is narrower in application, and makes the code harder to read.
Fixes #26292.
Fixes #29964.
Fixes #15074
Removes 238 bounds checks from std/cmd.
Change-Id: I1f87c32ee672bfb8be397b27eab7a4c2f304893f
Reviewed-on: https://go-review.googlesource.com/c/go/+/174704
Run-TryBot: Zach Jones <zachj1@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Giovanni Bajo <rasky@develer.com>
Sergei Zagurskii [Wed, 22 May 2019 14:00:34 +0000 (17:00 +0300)]
reflect: optimize directlyAssignable to avoid rtype.Name call
directlyAssignable invoked rtype.Name() just to compare its result
to empty string. We really only need to check whether rtype has
name. It can be done much cheaper, by checking tflagNamed.
Benchmark: https://play.golang.org/p/V2BzESPuf2w
name old time/op new time/op delta
DirectlyAssignable-12 32.7ns ± 6% 6.6ns ± 6% -79.80% (p=0.008 n=5+5)
Jason A. Donenfeld [Mon, 26 Aug 2019 13:15:34 +0000 (07:15 -0600)]
ld: fix up header copy and paste error
Some constants were added above the initial copyright blurb, and then
later a new copyright blurb was added on top of that. So we wound up
with two header sections, one of which contained a useful comment that
became obscured. This commit fixes up that mistake.
Change-Id: I8b9b8c34495cdceae959e151e8ccdee3137f6ca4
Reviewed-on: https://go-review.googlesource.com/c/go/+/191841
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Robert Griesemer [Fri, 23 Aug 2019 04:27:33 +0000 (21:27 -0700)]
go/types: process each segment of delayed actions in FIFO order
The stack of delayed actions is grown by pushing a new action
on top with Checker.later. Checker.processDelayed processes
all actions above a top watermark and then resets the stack
to top.
Until now, pushed actions above the watermark were processed
in LIFO order. This change processes them in FIFO order, which
seems more natural (if an action A was delayed before an action
B, A should be processed before B for that stack segment).
(With this change, Checker.later could be used instead of
Checker.atEnd to postpone interface method type comparison
and then the specific example in issue #33656 does type-check.
However, in general we want interface method type comparisons
to run after all interfaces are completed. With Checker.later
we may still end up mixing interface completions and interface
method type comparisons in ways leading to other errors for
sufficiently convoluted code.)
Also, move Checker.processDelayed from resolver.go to check.go.
Change-Id: Id31254605e6944c490eab410553fff907630cc64
Reviewed-on: https://go-review.googlesource.com/c/go/+/191458 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Robert Griesemer [Fri, 23 Aug 2019 00:03:30 +0000 (17:03 -0700)]
go/types: postpone interface method type comparison to the end
Introduce a new list of final actions that is executed at the
end of type checking and use it to collect method type comparisons
and also map key checks.
Fixes #33656.
Change-Id: Ia77a35a45a9d7eaa7fc3e9e19f41f32dcd6ef9d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/191418 Reviewed-by: Alan Donovan <adonovan@google.com>
When creating a new interface via the exported API calls, a shared
empty and completed Interface value is returned if there are no
methods or embedded interfaces. This is a minor optimization and
matches the internal behavior when creating empty interfaces.
Since calling Interface.Complete is idempotent, and since there
are no other legitimate ways to create Interface values externally
but via NewInterface/NewInterfaceType calls, and completed Interfaces
are considered "immutable", this change is not expected to affect
clients. The only observable behavior that changed is the string
value for empty interfaces created via the above API calls; those
empty interfaces now don't show "incomplete" anymore even before
Interface.Complete is called. Except in special test cases, this
behavior is unlikely to affect clients.
Change-Id: Idf7f2cd112241c5b81a43b4544bbe3f2e003d8d8
Reviewed-on: https://go-review.googlesource.com/c/go/+/191417 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Robert Griesemer [Thu, 22 Aug 2019 00:13:45 +0000 (17:13 -0700)]
go/types: allow embedding overlapping interfaces
Quietly drop duplicate methods from embedded interfaces
if they have an identical signature to existing methods.
Instead of adjusting the prior syntax-based only method set
computation where methods don't have signature information
(and thus where de-duplication according to the new rules
would have been somewhat tricky to get right), this change
completely rewrites interface method set computation, taking
a page from the cmd/compiler's implementation. In a first
pass, when type-checking interfaces, explicit methods and
embedded interfaces are collected, but the interfaces are
not "expanded", that is the final method set computation
is done lazily, either when needed for method lookup, or
at the end of type-checking.
While this is a substantial rewrite, it allows us to get
rid of the separate (duplicate and delicate) syntactical
method set computation and generally simplifies checking
of interface types significantly. A few (esoteric) test
cases now have slightly different error messages but all
tests that are accepted by cmd/compile are also accepted
by go/types.
(This is a replacement for golang.org/cl/190258.)
Updates #6977.
Change-Id: Ic8b9321374ab4f617498d97c12871b69d1119735
Reviewed-on: https://go-review.googlesource.com/c/go/+/191257 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Robert Griesemer [Tue, 13 Aug 2019 19:56:51 +0000 (12:56 -0700)]
go/types: add -halt flag to ease debugging in test mode
Specifying -halt in `go test -run Check$ -halt` causes a panic
upon encountering the first error. The stack trace is useful to
determine what code path issued the error.
Change-Id: I2e17e0014ba87505b01786980b98565f468065bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/190257 Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Kevin Gillette [Mon, 26 Aug 2019 03:30:21 +0000 (03:30 +0000)]
net/http: make docs refer to Context.Value as a getter instead of context.WithValue
The doc comments of both ServerContextKey and LocalAddrContextKey both suggest that context.WithValue can be used to access (get) properties of the server or connection. This PR fixes those comments to refer to Context.Value instead.
Cherry Zhang [Fri, 23 Aug 2019 18:40:20 +0000 (14:40 -0400)]
cmd/vendor: update vendored x/arch repo to 8a70ba74b3a1
Update vendored x/arch repo to pick up the fix of issue #33802.
This is done with the following commands:
$ cd $GOROOT/src/cmd
$ go get -d golang.org/x/arch@latest
go: finding golang.org/x/arch latest
go: downloading golang.org/x/arch v0.0.0-20190815191158-8a70ba74b3a1
go: extracting golang.org/x/arch v0.0.0-20190815191158-8a70ba74b3a1
$ go mod tidy
$ go mod vendor
http2: limit number of control frames in server send queue
An attacker could cause servers to queue an unlimited number of PING
ACKs or RST_STREAM frames by soliciting them and not reading them, until
the program runs out of memory.
Limit control frames in the queue to a few thousands (matching the limit
imposed by other vendors) by counting as they enter and exit the scheduler,
so the protection will work with any WriteScheduler.
Once the limit is exceeded, close the connection, as we have no way to
communicate with the peer.
Change-Id: I842968fc6ed3eac654b497ade8cea86f7267886b
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/525552 Reviewed-by: Brad Fitzpatrick <bradfitz@google.com>
This change was generated with cmd/go and cmd/bundle:
$ go get -u golang.org/x/net
$ go mod tidy
$ go mod vendor
$ go generate net/http
Fixes CVE-2019-9512 and CVE-2019-9514
Fixes #33606
Change-Id: I464baf96175006aa101d65d3b0f6494f28a626ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/190137 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Rob Pike [Thu, 22 Aug 2019 05:50:06 +0000 (15:50 +1000)]
strconv: simplify the text for bases in ParseInt
Followon from a review comment in https://golang.org/cl/191078
Change-Id: If115b2ae0df5e5cb9babd60802947ddb687d56c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/191219 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Jonathan Amsterdam [Thu, 22 Aug 2019 16:40:52 +0000 (12:40 -0400)]
syscall: document relationship among Errno, errors.Is and os.Err*
- Add doc to syscall.Errno (and syscall.ErrorString for plan9).
- Mention under `syscall` in release notes.
Fixes #33436.
Change-Id: I032ffebaa76ed67eb9d748e7645ca73f26144ea0
Reviewed-on: https://go-review.googlesource.com/c/go/+/191337 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Emmanuel T Odeke [Wed, 21 Aug 2019 05:09:31 +0000 (23:09 -0600)]
doc/go1.13: document _ between digits for math/big, strconv
Document that:
* math/big.Float.Parse
* math/big.Int.SetString
* strconv.ParseFloat
* strconv.ParseInt
* strconv.ParseUint
now accept underscores to group digits only if base = 0,
as per the Go 2 language changes.
Updates #32815
Change-Id: Id45bd803a18442436419739297e8aed0d32ca56c
Reviewed-on: https://go-review.googlesource.com/c/go/+/191077 Reviewed-by: Robert Griesemer <gri@golang.org>
Filippo Valsorda [Tue, 20 Aug 2019 21:29:04 +0000 (17:29 -0400)]
Revert "encoding/json: avoid work when unquoting strings"
This reverts CL 151157.
CL 151157 introduced a crash when decoding into ",string" fields. It
came with a moderate speedup, so at this stage of the release cycle
let's just revert it, and reapply it in Go 1.14 with the fix in CL 190659.
Also applied the test cases from CL 190659.
Updates #33728
Change-Id: Ie46e2bc15224b251888580daf6b79d5865f3878e
Reviewed-on: https://go-review.googlesource.com/c/go/+/190909
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>
Russ Cox [Wed, 21 Aug 2019 14:43:49 +0000 (10:43 -0400)]
cmd/go: accept GOSUMDB=sum.golang.google.cn
This CL makes the go command understand that
GOSUMDB=sum.golang.google.cn should connect
to that domain but expect to find a checksum database
signed by sum.golang.org there.
The host sum.golang.google.cn is not yet completely
configured; we hope it will be available in a few weeks.
Filippo Valsorda [Mon, 19 Aug 2019 17:17:34 +0000 (13:17 -0400)]
cmd/go: fix "go help build -o" docs
The docs refer to "the last two paragraphs", but in fact should refer to
the first two of the previous three paragraphs. Moved up the out of place
paragraph.
Updates #14295
Change-Id: I066da7a665bc6754d246782b941af214a385017a
Reviewed-on: https://go-review.googlesource.com/c/go/+/190839 Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Wagner Riffel [Mon, 19 Aug 2019 03:41:12 +0000 (00:41 -0300)]
doc: rewrite reference to plan9.bell-labs.com to 9p.io
Change-Id: I75619feced842b8ca509ee08e01b63258c5e87ca
Reviewed-on: https://go-review.googlesource.com/c/go/+/190757 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Dmitry Vyukov [Sat, 17 Aug 2019 20:22:22 +0000 (13:22 -0700)]
encoding/json: fix format string in the Fuzz func
Currently test build fails with:
$ go test -tags=gofuzz encoding/json
encoding/json/fuzz.go:36:4: Println call has possible formatting directive %s
FAIL encoding/json [build failed]
Change-Id: I23aef44a421ed0e7bcf48b74ac5a8c6768a4841b
Reviewed-on: https://go-review.googlesource.com/c/go/+/190698
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Joe Tsai [Fri, 16 Aug 2019 17:36:01 +0000 (10:36 -0700)]
time: update TestSub to avoid future regressions
CL 131196 optimized Time.Sub, but was reverted because
it incorrectly computed the nanoseconds in some edge cases.
This CL adds a test case to enforce the correct behavior
so that a future optimization does not break this again.
Updates #17858
Updates #33677
Change-Id: I596d8302ca6bf721cf7ca11cc6f939639fcbdd43
Reviewed-on: https://go-review.googlesource.com/c/go/+/190524
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Joe Tsai [Fri, 16 Aug 2019 17:42:18 +0000 (10:42 -0700)]
cmd/gofmt: update TestRewrite to avoid future regressions
CL 162337 changed go/ast to better handle block comments,
but was reverted because it introduced an off-by-one bug.
This CL adds a test case to enforce the correct behavior
so that future changes do not break this again.
Updates #18929
Updates #33538
Change-Id: I2d25c139d007f8db1091b7a48b1dd20c584e2699
Reviewed-on: https://go-review.googlesource.com/c/go/+/190523
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@golang.org>
Cherry Zhang [Wed, 14 Aug 2019 13:50:51 +0000 (09:50 -0400)]
reflect: align first argument in callMethod
When calling a function obtained from reflect.Value.Method (or
MethodByName), we copy the arguments from the caller frame, which
does not include the receiver, to a new frame to call the actual
method, which does include the receiver. Here we need to align
the first (non-receiver) argument. As the receiver is pointer
sized, it is generally naturally aligned, except on amd64p32,
where the argument can have larger alignment, and this aligning
becomes necessary.
Dmitri Shuralyov [Mon, 12 Aug 2019 19:40:50 +0000 (15:40 -0400)]
[dev.boringcrypto] misc/boring: remove download of releaselet.go in build.release
This step was added in CL 188738 to work around the issue
golang.org/issue/33443. That issue has now been resolved,
so this step is no longer needed and can be removed.
Filippo Valsorda [Tue, 6 Aug 2019 23:32:16 +0000 (19:32 -0400)]
net/url: make Hostname and Port predictable for invalid Host values
When Host is not valid per RFC 3986, the behavior of Hostname and Port
was wildly unpredictable, to the point that Host could have a suffix
that didn't appear in neither Hostname nor Port.
This is a security issue when applications are applying checks to Host
and expecting them to be meaningful for the contents of Hostname.
To reduce disruption, this change only aims to guarantee the following
two security-relevant invariants.
* Host is either Hostname or [Hostname] with Port empty, or
Hostname:Port or [Hostname]:Port.
* Port is only decimals.
The second invariant is the one that's most likely to cause disruption,
but I believe it's important, as it's conceivable an application might
do a suffix check on Host and expect it to be meaningful for the
contents of Hostname (if the suffix is not a valid port).
There are three ways to ensure it.
1) Reject invalid ports in Parse. Note that non-numeric ports are
already rejected if and only if the host starts with "[".
2) Consider non-numeric ports as part of Hostname, not Port.
3) Allow non-numeric ports, and hope they only flow down to net/http,
which will reject them (#14353).
This change adopts both 1 and 2. We could do only the latter, but then
these invalid hosts would flow past port checks, like in
http_test.TestTransportRejectsAlphaPort. Non-numeric ports weren't fully
supported anyway, because they were rejected after IPv6 literals, so
this restores consistency. We could do only the former, but at this
point 2) is free and might help with manually constructed Host values
(or if we get something wrong in Parse).
Note that net.SplitHostPort and net.Dial explicitly accept service names
in place of port numbers, but this is an URL package, and RFC 3986,
Section 3.2.3, clearly specifies ports as a number in decimal.
net/http uses a mix of net.SplitHostPort and url.Parse that would
deserve looking into, but in general it seems that it will still accept
service names in Addr fields as they are passed to net.Listen, while
rejecting them in URLs, which feels correct.
This leaves a number of invalid URLs to reject, which however are not
security relevant once the two invariants above hold, so can be done in
Go 1.14: IPv6 literals without brackets (#31024), invalid IPv6 literals,
hostnames with invalid characters, and more.
Tested with 200M executions of go-fuzz and the following Fuzz function.
u, err := url.Parse(string(data))
if err != nil {
return 0
}
h := u.Hostname()
p := u.Port()
switch u.Host {
case h + ":" + p:
return 1
case "[" + h + "]:" + p:
return 1
case h:
fallthrough
case "[" + h + "]":
if p != "" {
panic("unexpected Port()")
}
return 1
}
panic("Host is not a variant of [Hostname]:Port")
Fixes CVE-2019-14809
Updates #29098
Change-Id: I7ef40823dab28f29511329fa2d5a7fb10c3ec895
Reviewed-on: https://go-review.googlesource.com/c/go/+/189258 Reviewed-by: Ian Lance Taylor <iant@golang.org>
K. "pestophagous" Heller [Mon, 5 Aug 2019 05:46:29 +0000 (22:46 -0700)]
doc/install-source: create distinction between steps that involve "git clone"
Prior doc implied that "git clone" was one way to obtain a go1.4
bootstrap toochain, but it did not state this outright. Further,
the doc did not make it explicit in the "Fetch the repository"
section that one must necessarily "git clone" a second time in
the (presumed-to-be-uncommon) case where "git clone" had already
been perfomed in the "compiler binaries" section.
Updates #33402
Change-Id: Id70a6587b6ee09aca13559d63868b75cb07dff1e
Reviewed-on: https://go-review.googlesource.com/c/go/+/188900 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Bryan C. Mills [Thu, 8 Aug 2019 22:09:54 +0000 (18:09 -0400)]
cmd/go: query each path only once in 'go get'
If we don't know whether a path is a module path or a package path,
previously we would first try a module query for it, then fall back to
a package query.
If we are using a sequence of proxies with fallback (as will be the
default in Go 1.13), and the path is not actually a module path, that
initial module query will fail against the first proxy, then
immediately fall back to the next proxy in the sequence — even if the
query could have been satisfied by some other (prefix) module
available from the first proxy.
Instead, we now query the requested path as only one kind of path.
If we query it as a package path but it turns out to only exist as a
module, we can detect that as a PackageNotInModuleError with an
appropriate module path — we do not need to issue a second query to
classify it.
Fixes #31785
Change-Id: I581d44279196e41d1fed27ec25489e75d62654e3
Reviewed-on: https://go-review.googlesource.com/c/go/+/189517
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Jay Conrod <jayconrod@google.com>
Jay Conrod [Wed, 7 Aug 2019 20:27:39 +0000 (16:27 -0400)]
cmd/go: improve 'go mod download' and 'go list -m' error messages
modload.ListModules now wraps errors as module.ModuleError as
appropriate. The resulting errors always include the module path and
will include the version, if known.
'go mod download' no longer ignores errors reported by ListModules.
Previously, it started requesting module info, go.mod, and zip. Those
requests would fail, overwriting the original failure. They were
usually less descriptive.
'go mod download' with a module not in the build list (and no version
query) is now an error. Previously, this was silently ignored.
Fixes #30743
Change-Id: Icee8c1c6c5240de135a8b6ba42d6bbcdb757cdac
Reviewed-on: https://go-review.googlesource.com/c/go/+/189323
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Alberto Donizetti [Wed, 7 Aug 2019 18:11:46 +0000 (20:11 +0200)]
doc: update list of archs supporting -race
Change-Id: Id0a55674a16671aaee99182d9096a9263f7a80b3
Reviewed-on: https://go-review.googlesource.com/c/go/+/189357 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Document goroutine label inheritance. Goroutine labels are copied upon
goroutine creation and there is a test enforcing this, but it was not
mentioned in the docstrings for `Do` or `SetGoroutineLabels`.
Add notes to both of those functions' docstrings so it's clear that one
does not need to set labels as soon as a new goroutine is spawned if
they want to propagate tags.
Updates #32223
Updates #23458
Change-Id: Idfa33031af0104b884b03ca855ac82b98500c8b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/189317 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Jay Conrod [Tue, 6 Aug 2019 16:44:30 +0000 (12:44 -0400)]
cmd/go: improve error message for missing import starting with cmd/
In modload.Import, confirm that the import path does not start with
"cmd/" before calling QueryPackage, which returns a less helpful
error.
In load.loadPackageData, don't wrap errors with "unknown import path".
The wrapped error should always include the import path, and it's also
repeated in the PackageError wrapper.
Fixes #31031
Change-Id: I071efa22e3842c62831d096f888a8006811fe724
Reviewed-on: https://go-review.googlesource.com/c/go/+/189157
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
Jonathan Amsterdam [Mon, 5 Aug 2019 14:14:40 +0000 (10:14 -0400)]
doc/go1.13.html: describe error-value additions
A brief description at the top the Standard Library section of the
changes to support error wrapping.
Fixes #33365.
Change-Id: Id5a3b2fe148d9bfb949f2cfc9e5d8a1613a0e219
Reviewed-on: https://go-review.googlesource.com/c/go/+/188798 Reviewed-by: Ian Lance Taylor <iant@golang.org>
Damien Neil [Fri, 2 Aug 2019 18:18:56 +0000 (11:18 -0700)]
os: don't consult Is methods on non-syscall error types
CL #163058 moves interpretation of platform-specific errors to the
syscall package. Package syscall errors implement an Is method which
os.IsPermission etc. consult. This results in an unintended semantic
change to the os package predicate functions: The following program
now prints 'true' where it used to print 'false':
package main
import "os"
type myError struct{ error }
func (e myError) Is(target error) bool { return target == os.ErrPermission }
func main() { println(os.IsPermission(myError{})) }
Change the os package error predicate functions to only examine syscall
errors, avoiding this semantic change.
This CL does retain one minor semantic change: On Plan9, os.IsPermission
used to return true for any error with text containing the string
"permission denied". It now only returns true for a syscall.ErrorString
containing that text.
Change-Id: I6b512b1de6ced46c2f1cc8d264fa2495ae7bf9f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/188817
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
Keith Randall [Fri, 2 Aug 2019 17:36:03 +0000 (10:36 -0700)]
cmd/compile: fix crash on write barrier insertion
The compiler can crash if the compiled code tries to
unconditionally read from a nil pointer. This should cause
the generated binary to panic, not the compiler.
which now uses the Request.Body's io.ReaderFrom implementation,
if available, and permits system level optimizations such as
"sendfile" to be used to transmit/upload the Body, which greatly
speeds up file uploads.
Updates #33396
Change-Id: I7b8315c4b3e57ad47bb9be2b0c838857875d4bd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/188457 Reviewed-by: Andrew Bonventre <andybons@golang.org>