Caching the invocation of lookExtensions on an absolute path in Command
and reusing the cached result in Start is only viable if Cmd.Path isn't
set to a different value after Command returns.
For #66586.
For #68314.
Fixes#68331.
Change-Id: I57007850aca2011b11344180c00faded737617b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/596875
Reviewed-by: qiu laidongfeng2 <2645477756@qq.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
(cherry picked from commit d0146bd85b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/596976
CL 512155 fixed#36768, but introduced #62596.
CL 527820 fixed#62596, but meant that the code failed to look up
file extensions on Windows for a relative path.
This CL fixes that problem by recording whether it has already
looked up file extensions.
This does mean that if Path is set manually then we do not update
it with file extensions, as doing that would be racy.
For #66586Fixes#66598
Change-Id: I9a0305d1e466c5e07bfbe442566ea12f5255a96e
GitHub-Last-Rev: dc3169f235
GitHub-Pull-Request: golang/go#67035
Reviewed-on: https://go-review.googlesource.com/c/go/+/581695
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit 5532427c4b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/594495
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
This reapplies CL 512155, which was previously reverted in CL 527337.
The race that prompted the revert should be fixed by CL 527820,
which will be submitted before this one.
For #36768.
Updates #62596.
Change-Id: I3c3cd92470254072901b6ef91c0ac52c8071e0a2
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-race,gotip-windows-amd64-race,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/528038
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
- Ignore empty entries in PATH, like PowerShell does.
- If we resolve a path using an explicit relative entry in PATH,
treat it the same as we do for the implicit "." search path,
by allowing a later (absolute) PATH entry that resolves to the
same executable to return the absolute version of its path.
- If the requested path does not end with an extension matching
PATHEXT, return ErrNotFound (indicating that we potentially searched
for multiple alternatives and did not find one) instead of
ErrNotExist (which would imply that we know the exact intended path
but couldn't find it).
Fixes#61493.
Change-Id: I5b539d8616e3403825749d8eccf46725fa808a17
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-race,gotip-windows-amd64-race,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/528037
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
- Use the test binary itself for printing paths instead of building a
separate binary and running it through additional subprocesses.
- Factor out a common chdir helper.
- Use t.Setenv where appropriate.
- Reduce indirection in test helpers.
- Set NoDefaultCurrentDirectoryInExePath consistently in the
environment.
Also add a test case demonstrating an interesting behavior for
relative paths that may interact with #62596.
Fixes#62594.
For #62596.
Change-Id: I19b9325034edf78cd0ca747594476cd7432bb451
Reviewed-on: https://go-review.googlesource.com/c/go/+/528035
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This reverts CL 512155.
Reason for revert: CL 512155 introduced a race in that it caused
cmd.Start to set cmd.Path. Previously it was fine if code looked
at cmd.Path in one goroutine while calling cmd.Start in a different
goroutine.
A test case for this race is in CL 527495.
Change-Id: Ic18fdadf6763727f8ea748280d5f0e601b9bf374
Reviewed-on: https://go-review.googlesource.com/c/go/+/527337
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Change-Id: I4d755e401acf670fb5a154ff59e4e4335ed2138e
GitHub-Last-Rev: a91d74ae55
GitHub-Pull-Request: golang/go#62150
Reviewed-on: https://go-review.googlesource.com/c/go/+/520918
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
The built-in function `min` has been implemented and can now be used to replace some manually written `minType` helper functions.
Change-Id: Ie8ffc7881c8652ece752751214f1242bf76a6e7e
GitHub-Last-Rev: 5db344f131
GitHub-Pull-Request: golang/go#60866
Reviewed-on: https://go-review.googlesource.com/c/go/+/504315
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: qiulaidongfeng <2645477756@qq.com>
We cancel the Context to unblock the test as soon as all of the "exit"
processes have completed. If that happens to occur before all of the
"hang" processes have started, the Start calls may fail with
context.Canceled.
Since those errors are possible in normal operation of the test,
ignore them.
Fixes#61277.
Updates #61080.
Change-Id: I20db083ec89ca88eb085ceb2892b9f87f83705ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/508755
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
In CL 421441, we changed syscall to allow concurrent calls to
forkExec.
On platforms that support the pipe2 syscall that is the right
behavior, because pipe2 atomically opens the pipe with CLOEXEC already
set.
However, on platforms that do not support pipe2 (currently aix and
darwin), syscall.forkExecPipe is not atomic, and the pipes do not
initially have CLOEXEC set. If two calls to forkExec proceed
concurrently, a pipe intended for one child process can be
accidentally inherited by the other. If the process is long-lived, the
pipe can be held open unexpectedly and prevent the parent process from
reaching EOF reading the child's status from the pipe.
Fixes#61080.
Updates #23558.
Updates #54162.
Change-Id: I83edcc80674ff267a39d06260c5697c654ff5a4b
Reviewed-on: https://go-review.googlesource.com/c/go/+/507355
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Windows UTF-16 strings can contain unpaired surrogates, which can't be
decoded into a valid UTF-8 string. This file defines a set of functions
that can be used to encode and decode potentially ill-formed UTF-16
strings by using the
[the WTF-8 encoding](https://simonsapin.github.io/wtf-8/).
WTF-8 is a strict superset of UTF-8, i.e. any string that is
well-formed in UTF-8 is also well-formed in WTF-8 and the content
is unchanged. Also, the conversion never fails and is lossless.
The benefit of using WTF-8 instead of UTF-8 when decoding a UTF-16
string is that the conversion is lossless even for ill-formed
UTF-16 strings. This property allows to read an ill-formed UTF-16
string, convert it to a Go string, and convert it back to the same
original UTF-16 string.
Fixes#59971
Change-Id: Id6007f6e537844913402b233e73d698688cd5ba6
Reviewed-on: https://go-review.googlesource.com/c/go/+/493036
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Paul Hampson <Paul.Hampson@Pobox.com>
A //go:debug line mentioning an unknown or retired setting
should be diagnosed as making the program invalid. Do that.
We agreed on this in the proposal but I forgot to implement it.
Change-Id: Ie69072a1682d4eeb6866c02adbbb426f608567c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/476280
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
For #58141
Co-authored-by: Richard Musiol <neelance@gmail.com>
Co-authored-by: Achille Roussel <achille.roussel@gmail.com>
Co-authored-by: Julien Fabre <ju.pryz@gmail.com>
Co-authored-by: Evan Phoenix <evan@phx.io>
Change-Id: I52e3e161f81dcbe8605570e47d732992979c4d34
Reviewed-on: https://go-review.googlesource.com/c/go/+/479623
Run-TryBot: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
To allow using testenv.SyscallIsNotSupported, rewrite the test to use
the exported API only. Given that path is an absolute path,
exec.LookPath is equivalent to exec.findExecutable on linux.
Fixes#59087
Change-Id: Ia01b84d4e9d5a65a88dd995f9e3c8a81c4ccd19f
Reviewed-on: https://go-review.googlesource.com/c/go/+/481620
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Bypass: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
internal/platform.MustLinkExternal is used in various places to
determine whether external linking is required. It should always
match what the linker actually requires, but today does not match
because the linker imposes additional constraints.
Updates #31544.
Change-Id: I0cc6ad587e95c607329dea5d60d29a5fb2a9e722
Reviewed-on: https://go-review.googlesource.com/c/go/+/472515
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Change-Id: Ica8d5e5799a4de532764ae86cdb623508d3a8e18
GitHub-Last-Rev: 3e97cca9de
GitHub-Pull-Request: golang/go#58689
Reviewed-on: https://go-review.googlesource.com/c/go/+/471021
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
testenv.Command sets a default timeout based on the test's deadline
and sends SIGQUIT (where supported) in case of a hang.
Change-Id: I84d469d12ca1f39175b713ca41e7cd2202d11d96
Reviewed-on: https://go-review.googlesource.com/c/go/+/451895
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Allow GODEBUG users to report how many times a setting
resulted in non-default behavior.
Record non-default-behaviors for all existing GODEBUGs.
Also rework tests to ensure that runtime is in sync with runtime/metrics.All,
and generate docs mechanically from metrics.All.
For #56986.
Change-Id: Iefa1213e2a5c3f19ea16cd53298c487952ef05a4
Reviewed-on: https://go-review.googlesource.com/c/go/+/453618
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
In CL 436655 I added a GODEBUG setting to this test process to verify
that Wait is eventually called for every exec.Cmd before it becomes
unreachable. However, the cmdHang test helpers in
TestWaitInterrupt/Exit-hang and TestWaitInterrupt/SIGKILL-hang
intentially leak a subprocess in order to simulate a leaky third-party
program, as Go users might encounter in practical use.
To avoid tripping over the leak check, we call Wait on the leaked
subprocess in a background goroutine. Since we expect the process
running cmdHang to exit before its subprocess does, the call to Wait
should have no effect beyond suppressing the leak check.
Fixes#57596.
Updates #52580.
Updates #50436.
Change-Id: Ia4b88ea47fc6b605c27ca6d9d7669c874867a900
Reviewed-on: https://go-review.googlesource.com/c/go/+/460998
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
I made this test parallel in CL 439196, which exposed it to the
fork/exec race condition described in #22315. The ETXTBSY errors from
that race should resolve on their own, so we can simply retry the call
to get past them.
Fixes#56811.
Updates #22315.
Change-Id: I2c6aa405bf3a1769d69cf08bf661a9e7f86440b4
Reviewed-on: https://go-review.googlesource.com/c/go/+/458016
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
We have been expanding our use of GODEBUG for compatibility,
and the current implementation forces a tradeoff between
freshness and efficiency. It parses the environment variable
in full each time it is called, which is expensive. But if clients
cache the result, they won't respond to run-time GODEBUG
changes, as happened with x509sha1 (#56436).
This CL changes the GODEBUG API to provide efficient,
up-to-date results. Instead of a single Get function,
New returns a *godebug.Setting that itself has a Get method.
Clients can save the result of New, which is no more expensive
than errors.New, in a global variable, and then call that
variable's Get method to get the value. Get costs only two
atomic loads in the case where the variable hasn't changed
since the last call.
Unfortunately, these changes do require importing sync
from godebug, which will mean that sync itself will never
be able to use a GODEBUG setting. That doesn't seem like
such a hardship. If it was really necessary, the runtime could
pass a setting to package sync itself at startup, with the
caveat that that setting, like the ones used by runtime itself,
would not respond to run-time GODEBUG changes.
Change-Id: I99a3acfa24fb2a692610af26a5d14bbc62c966ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/449504
Run-TryBot: Russ Cox <rsc@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
As the comments say. Here we expect err to be nil instead of ctx.Err()
Change-Id: I4cd02d62ac0a13c9577a567de36742f13d140d36
GitHub-Last-Rev: 6bedfbc9d2
GitHub-Pull-Request: golang/go#56698
Reviewed-on: https://go-review.googlesource.com/c/go/+/449737
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Plan 9 uses NUL as os.PathListSeparator, so it's almost always going
to appear in the environment variable list. Exempt GOOS=plan9 from the
check for NUL in environment variables.
For #56284.
Fixes#56544.
Change-Id: I23df233cdf20c0a9a606fd9253e15a9b5482575a
Reviewed-on: https://go-review.googlesource.com/c/go/+/447715
Reviewed-by: David du Colombier <0intro@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
In https://build.golang.org/log/d2eb315305bf3d513c490e7f85d56e9a016aacd2,
we observe a failure in TestPipeLookPathLeak due to an additional
descriptor (7) that was open at the start of the test being closed while
the test executes.
I haven't dug much into the failure, but it seems plausible to me that the
descriptor may have been opened by libc for some reason, and may have been
closed due to some sort of idle timeout or the completion of a background
initialization routine.
Since the test is looking for a leak, and closing an existing descriptor
does not indicate a leak, let's not fail the test if an existing descriptor
is unexpectedly closed.
Updates #5071.
Change-Id: I03973ddff6592c454cfcc790d6e56accd051dd52
Reviewed-on: https://go-review.googlesource.com/c/go/+/447235
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Check for and reject environment variables containing NULs.
The conventions for passing environment variables to subprocesses
cause most or all systems to interpret a NUL as a separator. The
syscall package rejects environment variables containing a NUL
on most systems, but erroniously did not do so on Windows. This
causes an environment variable such as "FOO=a\x00BAR=b" to be
interpreted as "FOO=a", "BAR=b".
Check for and reject NULs in environment variables passed to
syscall.StartProcess on Windows.
Add a redundant check to os/exec as extra insurance.
Fixes#56284
Fixes CVE-2022-41716
Change-Id: I2950e2b0cb14ebd26e5629be1521858f66a7d4ae
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1609434
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/446916
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
If we use the "pipetest" helper command instead of "sleep",
we can use its stdout pipe to determine when the process
is ready to handle a SIGSTOP, and we can additionally check
that sending a SIGCONT actually causes the process to continue.
This also allows us to remove the "sleep" helper command,
making the test file somewhat more concise.
Noticed while looking into #50138.
Change-Id: If4fdee4b1ddf28c6ed07ec3268c81b73c2600238
Reviewed-on: https://go-review.googlesource.com/c/go/+/442576
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
This will dump more goroutines if the test happens to fail.
For #50138.
Change-Id: Ifae30b5ba8bddcdaa9250dd90be8d8ba7d5604d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/442476
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
As of CL 438347, multiple concurrents calls to Close should be safe.
This removes some indirection and may also make some programs that use
type-assertions marginally more efficient. For example, if a program
calls (*exec.Cmd).StdinPipe to obtain a pipe and then sets that as the
Stdout of another command, that program will now allow the second
command to inherit the file descriptor directly instead of copying
everything through a goroutine.
This will also cause calls to Close after the first to return an error
wrapping os.ErrClosed instead of nil. However, it seems unlikely that
programs will depend on that error behavior: if a program is calling
Write in a loop followed by Close, then if a racing Close occurs it is
likely that the Write would have already reported an error. (The only
programs likely to notice a change are those that call Close — without
Write! — after a call to Wait.)
Updates #56043.
Updates #9307.
Updates #6270.
Change-Id: Iec734b23acefcc7e7ad0c8bc720085bc45988efb
Reviewed-on: https://go-review.googlesource.com/c/go/+/439195
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Wait or Run will populate its ProcessState when the command completes.
Fixes#56002.
Change-Id: I21547431f5d2d3e0fc0734fd1705421a0ac4209c
Reviewed-on: https://go-review.googlesource.com/c/go/+/437996
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
This cuts the wall duration for 'go test os/exec' and
'go test -race os/exec' roughly in half on my machine,
which is an even more significant speedup with a high '-count'.
For better or for worse, it may also increase the repro rate
of #34988.
Tests that use Setenv or Chdir or check for FDs opened during the test
still cannot be parallelized, but they are only a few of those.
Change-Id: I8d284d8bff05787853f825ef144aeb7a4126847f
Reviewed-on: https://go-review.googlesource.com/c/go/+/439196
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
This test has been disabled for over nine years (since CL 12869049).
Although it still compiles, it seems likely to have rotted since then,
and if it was going to detect a real bug it also seems like that bug
would have been encountered and reported by users since then (and
would presumably have its own regression tests).
To eliminate overhead from mainining it (or skipping over it while
maintaining other tests), let's just delete it.
Fixes#5780.
Change-Id: I2a85cba20cba98a1dc6fc82336ae5e22d2242e99
Reviewed-on: https://go-review.googlesource.com/c/go/+/439197
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This reverts CL 437176.
Reason for revert: broke programs that plumb StdoutPipe from one command to Stdin on another and then call Wait on the former.
os/exec itself uses a type-assertion to *os.File to determine whether to copy stdin using a goroutine or just pass a file descriptor. An early Wait using a *os.File is benign (because closing the pipe doesn't close the child's inherited file descriptor), but an early Wait using a non-*os.File is not.
Updates #50436.
Change-Id: I4a2993e290982834f91696d890dfe77364c0cc50
Reviewed-on: https://go-review.googlesource.com/c/go/+/438695
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
For #50436, I want to be able to close the pipes returned by
StdoutPipe and StderrPipe after the Context has been canceled
and the WaitDelay has subsequently expired.
However, the fact that the exec.onceCloser wrapper for StdinPipe
(added in CL 13329043) was retained in CL 65490 suggests to me that
(*os.File).Close is still not safe to call concurrently.
This may cause type assertions of these ReadClosers to *os.File that
once succeeded to no longer do so. However, the StdoutPipe and
StderrPipe methods return interfaces, not concrete *os.Files, so
callers already should not have been relying on that implementation
detail — and as far as I can tell the closeOnce wrapper does not mask
any (*os.File) methods, so assertions to any interface type that
previously succeeded will continue to do so.
This change is logically part of CL 401835, but since it may expose
fragile type-assertions in callers I want to keep it separate for
clearer bisection of any new test failures.
For #50436.
Change-Id: I58de1d48fb6fd788502f13657d8d4484516271cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/437176
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
This change undoes CL 401894, because on further consideration
it turns out not to be needed.
This also makes (*Cmd).closeDescriptors a free function, since it does
not actually use the receiver in any way and is not needed to satisfy
any interfaces.
For #50436.
Change-Id: I7915265b0e6398ed5a34fae0c12873ab08a14194
Reviewed-on: https://go-review.googlesource.com/c/go/+/437175
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Use a separate channel to report the final error of the copying
goroutines, receiving a value only when all of the copying goroutines
have completed. In a followup change (CL 401835), that will allow
waiters to select on goroutine completion alongside other events (such
as Context cancellation).
Also mildly refactor the watchCtx helper method so that its structure
better matches what will be needed to implement WaitDelay.
For #50436.
Change-Id: I54b3997fb6931d204814d8382f0a388a67b520f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/435995
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
This fixes a bug introduced in CL 401834 in which calling Start twice
with pipes attached to a command would spuriously close those pipes
when returning the error from the second Start call.
For #50436.
Change-Id: I3563cc99c0a0987752190fef95da3e9927a76fda
Reviewed-on: https://go-review.googlesource.com/c/go/+/436095
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
The pipes pumped by goroutines can be closed as soon as their
respective goroutines are done.
The pipes pumped by user code, however, are documented to be closed in
Wait. When we add the WaitDelay field, it isn't obvious that we should
terminate the user-pumped pipes when the WaitDelay expires, since Wait
itself isn't going to wait for those user-controlled goroutines to
complete.
(It's a bit more complicated than that because the documentation
currently states that Wait must not be called while the pipes are
being read — but it isn't obvious to me that that advice is entirely
correct.)
For #50436.
Change-Id: I97909c91d2097fb75138a360747168c08609696d
Reviewed-on: https://go-review.googlesource.com/c/go/+/401894
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Change the childFiles field to a local variable, since it was
populated during Start and (as far as I can determine) has no purpose
after Start returns.
Rename closeAfterStart and closeAfterWait to childIOFiles and
parentIOPipes respectively. That makes their contents clearer, and also
helps to clarify what should happen on error (when, for example, Wait
shouldn't be called at all).
Use a deferred call instead of individual calls to close child (and,
if necessary, pipe) FDs after Start. That helps to clarify the
invariants around when they are closed, and also makes the function a
bit more robust for future refactoring.
Also nil out the slices containing the file closers so that they can
be collected earlier.
This CL is intended as a pure refactor in preparation for #50436.
Change-Id: I05d13fa91d539b95b84b2ba923c1733f9a6203e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/401834
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Apparently, some testing environments do not allow root to mount tmpfs
(due to e.g. AppArmor profile disallowing mount(2) syscall).
Always skip the test if the mount has failed.
Fixes the test issue introduced by CL 414824.
Change-Id: Ic565d2e6f277f2926d85a351be7df2498ffba656
Reviewed-on: https://go-review.googlesource.com/c/go/+/429175
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Change-Id: I610509aa35c345ff7fbb1fc94bf177ffbe934731
Reviewed-on: https://go-review.googlesource.com/c/go/+/428274
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Having an executable bit set for a binary is not enough for it to be
executable -- there might be more checks in the kernel. For example,
binaries on a filesystem mounted with "noexec" flag couldn't be
executed. There might be other scenarios involving ACLs, SELinux,
file capabilities, and so on.
As a result, LookPath might either find a non-executable (while going
over $PATH elements), or return a false positive that the argument
provided is an executable.
One possible fix would be to perform the check by using access(2)
syscall with X_OK flag.
Now, since access(2) uses real (rather than effective) uid and gid,
when used by a setuid or setgid binary, it checks permissions of the
(real) user who started the binary, rather than the actual effective
permissions. Therefore, using access with X_OK won't work as expected
for setuid/setgid binaries.
To fix this, modern platforms added ways to check against effective uid
and gid, with the most common being the faccessat(2) call with the
AT_EACCESS flag, as described by POSIX.1-2008 (in Linux, only
faccessat2(2) supports flags such as AT_EACCESS). Let's use it, and fall
back to checking permission bits if faccessat is not available.
Wrap the logic into unix.Eaccess, which is currently only implemented on
Linux. While many other OSes (Free/Net/OpenBSD, AIX, Solaris/Illumos, and
Darwin) do implement faccessat(2) with AT_EACCESS, it is not wired in
syscall package (except for AIX), so these platforms are left out for now.
In the future, eaccess can be implemented for these OSes, too.
Alas, a call to unix.Eaccess is not enough since we have to filter out
directories, so use both stat and Eaccess.
One minor change introduced by this commit is that LookPath and Command
now returns "is a directory" error when the argument contains a slash
and is a directory. This is similar to what e.g. bash does on Linux:
$ bash -c /etc
bash: line 1: /etc: Is a directory
Add a test case, which, unfortunately, requires root, is specific to
Linux, and needs a relatively new kernel (supporting faccessat2). Other
platforms either have different semantics for tmpfs with noexec, or have
different ways to set up a binary which has x bit set but nevertheless
could not be executed.
Change-Id: If49b6ef6bf4dd23b2c32bebec8832d83e511a4bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/414824
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
The changes are likely to break users, and we need
to make it easy to unbreak without code changes.
For #43724.
Fixes#53962.
Change-Id: I105c5d6c801d354467e0cefd268189c18846858e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419794
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Dmitri Shuralyov
qiulaidongfeng
Tobias Klauser
Bryan C. Mills
Ian Lance Taylor
Jes Cok
apocelipes
qmuntal
Russ Cox
Johan Brandhorst-Satzkorn
pgxiaolianzi
cui fliter
scott
Matthew Dempsky
Damien Neil
hopehook
Kir Kolyshkin
cuiweixie