Skip to content
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

extended backwards compatibility for Go #56986

Closed
rsc opened this issue Nov 29, 2022 · 67 comments
Closed

extended backwards compatibility for Go #56986

rsc opened this issue Nov 29, 2022 · 67 comments

Comments

@rsc
Copy link
Contributor

rsc commented Nov 29, 2022

Go's emphasis on backwards compatibility is one of its key strengths.
There are, however, times when we cannot maintain strict compatibility,
such as when changing sort algorithms or fixing clear bugs,
when existing code depends on the old algorithm or the buggy behavior.
This proposal aims to address many such situations by keeping older Go programs
executing the same way even when built with newer Go distributions.

See my talk on this topic at GopherCon for background.

See the design document for details.

@rsc rsc added the Proposal label Nov 29, 2022
@rsc rsc added this to the Proposal milestone Nov 29, 2022
@gopherbot
Copy link

Change https://go.dev/cl/453605 mentions this issue: cmd/go: set default GODEBUG for main packages

@gopherbot
Copy link

Change https://go.dev/cl/453604 mentions this issue: cmd/go: parse and report directives in file headers

@gopherbot
Copy link

Change https://go.dev/cl/453603 mentions this issue: go/build: parse and report directives in file headers

@rsc
Copy link
Contributor Author

rsc commented Nov 29, 2022

Design doc at https://go.dev/design/56986-godebug.

@Merovius
Copy link
Contributor

An unrecognized //go:debug setting is a build error. It is also an error

Syntax error: Unterminated sentence (I assume).

@rittneje
Copy link

It would also be nice if runtime/debug.BuildInfo reported all the //go:debug directives that got compiled in.

@mateusz834
Copy link
Contributor

mateusz834 commented Nov 30, 2022

@rittneje 
Note that there is still a possibility that the compiled //go:debug lines might change at runtime (GODEBUG env vars). 

@rsc 
We should probably also assume that someone in the wild is also parsing the GODEBUG, to detect additional capabilities (like sha1 in certs). Maybe we could add an interface to access them:

  1. compiled ones like proposed by @rittneje (through BuildInfo) and
  2. merge of compiled and GODEBUG env var (somewhere inside runtime package)

Or (And?) the runtime might re-set the GODEBUG env variable to the merged one.

@rsc
Copy link
Contributor Author

rsc commented Nov 30, 2022

@rittneje, great idea, thanks, added to the proposal and the implementation sketch as a new Setting with key "DefaultGODEBUG".

@mateusz834 People in the wild should really not be parsing GODEBUG. But if they really want to, they can now use the BuildInfo setting to find the default and then consult os.Getenv("GODEBUG") for the overrides. Changing the environment variable to hold the entire unified setting is a mistake because it would affect subprocesses.

@Merovius, fixed, thanks.

@liggitt
Copy link
Contributor

liggitt commented Nov 30, 2022

Thanks, the design lgtm. I just had a couple thoughts as I read through it:

Set the default GODEBUG settings based on the go line the main module's go.mod, so that updating to a new Go toolchain with an unmodified go.mod mimics the older release.

Can the design add details about what this will do when building a main module whose go.mod references a go version prior to this compatibility behavior? For example, if this capability arrives in go1.21, when building a main module whose go.mod contains go 1.17, would go1.21 attempt to default to 1.17- or 1.20-compatible godebug settings?

An unrecognized //go:debug setting is a build error.

This means that when eventually-removed godebug options are dropped (after 2+ years / 4+ releases), a program explicitly reverting a runtime behavior with this option will no longer build with new go versions. This seems reasonable to me (even preferable compared to silently ignoring an explicit go:debug directive) but should be explicitly documented:

  • to clarify to a user toggling this on that this represents a future incompatibility for them, and the limits of the compatibility guarantee for programs using //go:debug
  • to encourage go maintainers to keep a high bar for making breaking changes even with this formalized extended compatibility approach

@rsc
Copy link
Contributor Author

rsc commented Nov 30, 2022

@liggitt, if this lands in Go 1.21, then I think probably we would make any go line prior to Go 1.20 provide Go 1.20 semantics, so that updating from a Go 1.20 toolchain to a Go 1.21 toolchain does not downgrade behaviors. The current sketch puts the baseline at Go 1.19, in part just to have a GODEBUG (randautoseed) to infer. A case could be made for Go 1.19 for people updating from Go 1.19 to Go 1.21, but I think the 1.20 -> 1.21 transitions should probably be given priority.

I agree with your comments about the unrecognized //go:debug lines. It is worth pointing out that these can only appear in a main package, so at least there is no chance of dependencies causing problems.

@davecb
Copy link

davecb commented Nov 30, 2022

Has there been a discussion of the extent to which person wishing to defeat security could use a GODEBUG environment variable to, for example, use SHA1 certificates?

@liggitt
Copy link
Contributor

liggitt commented Nov 30, 2022

@liggitt, if this lands in Go 1.21, then I think probably we would make any go line prior to Go 1.20 provide Go 1.20 semantics, so that updating from a Go 1.20 toolchain to a Go 1.21 toolchain does not downgrade behaviors.

That matches my expectations. I agree we should not downgrade behaviors simply by updating the toolchain.

Anyone updating their toolchain from earlier go versions (which would not get the automatic go.mod-informed defaulting) who needs to maintain compatibility with specific still-supported godebug options could use explicit //go:debug lines to do so in a non-invasive way.

Mentioning both of those points in the design/documentation would be helpful.

@mateusz834
Copy link
Contributor

mateusz834 commented Nov 30, 2022

Should we warn users when compiling code with GODEBUG settings enabled?
Let's say that some linux distribution compiles Go tools and the autor of one of the tool set: //go:debug sha1=1. When package maintainers are compiling the code they will have no idea that something inseucre is being enabled. Till now we had to mark it explicitly that we want something insecure (by using GODEBUG env var).
Maybe we shouldn't warn about all GODEBUG, only ones related to security (sha1=1,x509ignoreCN=0,execerrdot=0)
We might also extend the //go:debug syntax to add a author-provided reasoning behind using it, so that it will be included in the warning.

Edit: probably only in go install and go build, for go run and go test it would be annoying for developers.

@rittneje
Copy link

Guarantee that GODEBUG settings last for at least 2 years (4 releases).

Set the default GODEBUG settings based on the go line the main module's go.mod, so that updating to a new Go toolchain with an unmodified go.mod mimics the older release.

What happens if my go.mod references an older Go version such that it is no longer possible to maintain compatibility because some of the requisite GODEBUG flags are no longer supported?

@rsc
Copy link
Contributor Author

rsc commented Nov 30, 2022

@davecb: If someone wants to defeat security by setting environment variables, PATH is often a better choice than GODEBUG. In any event, we already have the setting, so this proposal does not make any environment-based vulnerabilities worse.

@mateusz834: I don't believe we should warn users during a build or install with GODEBUG settings. They're just a part of the program. There are plenty of ways a program can be made insecure. It would be strange to warn about just this one. (We don't warn users during a build or install of programs importing "crypto/sha1" either.)

@rittneje: If go.mod mentions an old Go version for which some of the necessary GODEBUGs have been retired, then the build proceeds as before. Most programs that say 'go 1.14' don't need SHA1 certificates, so even if the SHA1 GODEBUG has been retired, the build should proceed in the most compatible way it can. The balance comes out differently in programs that say //go:debug x509sha1=1. Those are saying very clearly that they do need SHA1, so if we can't provide that, the build should fail.

@liggitt: I will update the rationale to reflect this conversation. Thanks.

@willfaught
Copy link
Contributor

Russ Cox, Michael Matloob, and Bryan Millls will do the work.

I assume Millls -> Mills?

@rsc
Copy link
Contributor Author

rsc commented Nov 30, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@beoran
Copy link

beoran commented Dec 3, 2022

I hate to bikeshed but I never heard about GODEBUG before and I think it's not a very good name for enabling backwards compatible behavior. GOCOMPAT, could perhaps be better?

@ianlancetaylor
Copy link
Contributor

Perhaps GOCOMPAT would be slightly better, but we are already using GODEBUG, and it's fine. No reason to change at this point.

@zephyrtronium
Copy link
Contributor

For reference, the name was previously discussed at #55090 (comment), including a suggestion of GOCOMPAT.

gopherbot pushed a commit to golang/proposal that referenced this issue Dec 6, 2022
Based on discussion on golang/go#56986 with Jordan Liggitt.

Change-Id: I7787155ea8194d879cadf0e0a9d043dd2ef5c38f
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/455316
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/455316 mentions this issue: design/56986-godebug: add notes about transition and old GODEBUGs

@rsc
Copy link
Contributor Author

rsc commented Dec 14, 2022

Does anyone have any concerns about accepting this proposal?

@rittneje
Copy link

rittneje commented Dec 14, 2022

@rsc One thing this proposal does not make clear is what happens if there are //go:debug directives outside the main package. Will the compiler give an error, or will they be silently ignored?

On a related note, how exactly will they work for unit testing? Will it respect //go:debug directives in the package under test, even if it isn't main? Should they go in a _test.go file? How will this interact with black box testing (i.e., package X_test)?

Also, I don't find this rationale convincing:

If go.mod mentions an old Go version for which some of the necessary GODEBUGs have been retired, then the build proceeds as before. Most programs that say 'go 1.14' don't need SHA1 certificates, so even if the SHA1 GODEBUG has been retired, the build should proceed in the most compatible way it can. The balance comes out differently in programs that say //go:debug x509sha1=1. Those are saying very clearly that they do need SHA1, so if we can't provide that, the build should fail.

Since the compiler cannot guess the intent, I believe the best thing it can do in this situation is fail (with a clear error message). Otherwise you can end up with an application that is subtly broken. The user has two options:

  1. Update the go directive in go.mod to a newer version that no longer implies the obsolete GODEBUG setting.
  2. Add an explicit //go:debug directive to the main package to opt out of the legacy behavior.

@rsc rsc added this to the Backlog milestone Jan 18, 2023
gopherbot pushed a commit to golang/tools that referenced this issue Jan 19, 2023
The directive analyzer is a generalized version of
the buildtag analyzer, meant to apply checks about
any directives (//go:... lines) found in source code.

For now it only checks the placement of //go:debug lines.

For golang/go#56986.

Change-Id: I2bd3d743c44554711ada90f6ee53b6195dc55bcb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/462216
Run-TryBot: Russ Cox <rsc@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/461956 mentions this issue: runtime: replace panic(nil) with panic(new(runtime.PanicNilError))

gopherbot pushed a commit that referenced this issue Jan 19, 2023
Long ago we decided that panic(nil) was too unlikely to bother
making a special case for purposes of recover. Unfortunately,
it has turned out not to be a special case. There are many examples
of code in the Go ecosystem where an author has written panic(nil)
because they want to panic and don't care about the panic value.

Using panic(nil) in this case has the unfortunate behavior of
making recover behave as though the goroutine isn't panicking.
As a result, code like:

	func f() {
		defer func() {
			if err := recover(); err != nil {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
	}

looks like it guarantees that call2 has been run any time f returns,
but that turns out not to be strictly true. If call1 does panic(nil),
then f returns "successfully", having recovered the panic, but
without calling call2.

Instead you have to write something like:

	func f() {
		done := false
		defer func() {
			if err := recover(); !done {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
		done = true
	}

which defeats nearly the whole point of recover. No one does this,
with the result that almost all uses of recover are subtly broken.

One specific broken use along these lines is in net/http, which
recovers from panics in handlers and sends back an HTTP error.
Users discovered in the early days of Go that panic(nil) was a
convenient way to jump out of a handler up to the serving loop
without sending back an HTTP error. This was a bug, not a feature.
Go 1.8 added panic(http.ErrAbortHandler) as a better way to access the feature.
Any lingering code that uses panic(nil) to abort an HTTP handler
without a failure message should be changed to use http.ErrAbortHandler.

Programs that need the old, unintended behavior from net/http
or other packages can set GODEBUG=panicnil=1 to stop the run-time error.

Uses of recover that want to detect panic(nil) in new programs
can check for recover returning a value of type *runtime.PanicNilError.

Because the new GODEBUG is used inside the runtime, we can't
import internal/godebug, so there is some new machinery to
cross-connect those in this CL, to allow a mutable GODEBUG setting.
That won't be necessary if we add any other mutable GODEBUG settings
in the future. The CL also corrects the handling of defaulted GODEBUG
values in the runtime, for #56986.

Fixes #25448.

Change-Id: I2b39c7e83e4f7aa308777dabf2edae54773e03f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/461956
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Jan 19, 2023
…rics

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>
@apparentlymart
Copy link

apparentlymart commented Jan 25, 2023

I realize I'm late to this discussion and I don't think what I'm about to write here ought to change the decision, but perhaps could tweak some details of how it works. I could write up a new proposal for this if that would be a better way to have this discussion, though I must admit I'm currently at the "problem statement" stage rather than the "proposed solution" stage.


I can see how using an environment variable to change behavior at runtime is pragmatic for the common situation where the same team is both developing and deploying an application: they fully control both the build environment and the runtime environment, so they can make sure that the runtime environment gels well with the assumptions the program is making.

I primarily work on applications that are shipped in binary form to end users who then run those binaries on their own systems. We cannot directly control the environment where these applications are run. Furthermore, the architecture of our system is that one program written in Go runs other separate programs that might also be written in Go but be written to target different versions of Go. Those child processes inherit the environment of the top-level process, and so all of these programs would typically end up sharing the same GODEBUG value if an end-user were to set it for some reason.

For these reasons I'm particularly sensitive to anything that allows subtle changes to implementation details of libraries our programs rely on without recompiling the program. In particular I'm worried about users either intentionally or unintentionally (e.g. because they are running other unrelated software written in Go) running our software with unusual GODEBUG settings that could then cause this software to malfunction or be vulnerable to a security problem it would otherwise be protected from. Relatedly (but less importantly), the cohort who doesn't know that GODEBUG was involved is unlikely to mention it when reporting a bug, thereby making it unclear how we can reproduce the bug they are reporting on our own systems.

I appreciate the inclusion of the option to force certain GODEBUG settings on via directives in the source code; that will be useful if we need to rely on backward-compatibility behaviors for a time after upgrading to a new version of Go.

The story would be complete for us if there were also an option to build a Go program in a way that makes it ignore the GODEBUG environment variable and rely exclusively on the directives specified inside the program. For our users the fact that the program is written in Go is an implementation detail and so we would never rely on dynamically-set Go runtime settings as a means for backward-compatibility, and so I'd like to be able to disable that mechanism altogether at build time so that we can always know exactly the behaviors that a particular release is relying on.

We fully control the entry point of our software so a new function in runtime/debug that we could call early in our main to disable use of GODEBUG for the rest of runtime would be sufficient for our needs, though I think anything that we can control entirely at build time would be suitable to meet this need.


If the above doesn't seem practical or reasonable, then we'll probably instead change our program to emit debug logs reporting the value of GODEBUG during early startup and hope that folks reporting bugs will include that part of the log to help us see if they have it set.

I saw some discussion above about it being okay to use os.Getenv("GODEBUG") to achieve than and this seems fine to me if it seems likely that this won't require ongoing tweaking and as this mechanism evolves. In an ideal world I'd like to see another function in runtime/debug that is guaranteed to return all dynamically-chosen features regardless of how they were chosen so that it doesn't feel like I'm depending on an implementation detail, but a compatibility promise for GODEBUG being the exclusive definition of runtime compatibility settings would suffice too.


I understand that it's technically already possible for behavior to vary at runtime because GODEBUG has been around for some time already. In some cases we intentionally explicitly disabled certain settings that we were aware of, but some of them are unintentionally available for our released software today. If this mechanism will be used more prolifically though, I would prefer a means to either control it exclusively at build time or to reliably report on it at runtime for diagnostic purposes.

Thanks!

@thepudds
Copy link
Contributor

thepudds commented Jan 25, 2023

Hi @apparentlymart. First, I can definitely empathize, including it can be that a support engineer for another product asked one of your users to introduce an environment change to work around an issue in the other product which then impacts your product.

It depends on your circumstances, but one approach can be to have a wrapper that launches your main binary, where the wrapper is responsible for massaging any environment variables. That wrapper could be another thin binary, or a shell script on UNIX, or a parent Windows service on Windows, and so on. However, that might be awkward or perhaps impossible depending what other constraints you have, including how your binary might be expected to fit into other bits of the ecosystem that might be outside your control.

One other comment is a new debug API that can be called by your code to control its own GODEBUG behavior might not be a full solution, including some of the settings might impact the runtime before any user code executes.

gopherbot pushed a commit that referenced this issue Jan 30, 2023
For #56986, add the new directive analyzer that catches
misplaced //go:debug lines.

Ran 'go mod vendor' after adding the import in vet
to bring in the vendored files.

A followup CL will enable it by default in 'go test'.

Change-Id: I12c46e292b31bdbf5ceb86ba4474545e78a83a47
Reviewed-on: https://go-review.googlesource.com/c/go/+/462201
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/464135 mentions this issue: cmd/go: enable vet directive analyzer during 'go test'

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
For golang#56986, add the new directive analyzer that catches
misplaced //go:debug lines.

Ran 'go mod vendor' after adding the import in vet
to bring in the vendored files.

A followup CL will enable it by default in 'go test'.

Change-Id: I12c46e292b31bdbf5ceb86ba4474545e78a83a47
Reviewed-on: https://go-review.googlesource.com/c/go/+/462201
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue Feb 23, 2023
For #56986, go/build needs to report up to cmd/go
about //go:debug lines found in the source code.
Rather than make a special case for //go:debug,
this change gathers all top-level directives above the
package line and includes them in the result.

The go command's module index must match go/build,
so this CL contains the code to update the index as well.

A future CL will use the //go:debug lines to prepare the default
GODEBUG settings, as well as rejecting such lines in non-main
packages.

Change-Id: I66ab8dc72f9cd65c503b10b744367caca233f8a6
Reviewed-on: https://go-review.googlesource.com/c/go/+/453603
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 23, 2023
For #56986, change the go command to compute and set the
default GODEBUG settings for each main package, based on
the work module's go version and the //go:debug lines in the
main package.

Change-Id: I2118cf0ae6d981138138661e02120c05af648872
Reviewed-on: https://go-review.googlesource.com/c/go/+/453605
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Feb 23, 2023
For #56986, run the new directive analyzer during 'go test',
to diagnose problems that would otherwise be missed,
like //go:debug appearing in the wrong place in a file
or in the wrong files.

Change-Id: I1ac230c3c67e58b5e584128e0ec6ff482cb225f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/464135
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/499415 mentions this issue: doc/go1.21: mention directive handling in go/{ast,build}

gopherbot pushed a commit that referenced this issue May 31, 2023
For #56986
For #59033

Change-Id: I7d03fe34d418aff97a551b236b5d43506e402871
Reviewed-on: https://go-review.googlesource.com/c/go/+/499415
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@mknyszek mknyszek mentioned this issue Jun 1, 2023
420 tasks
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 4, 2023
Nasfame pushed a commit to golangFame/go that referenced this issue Jun 4, 2023
For golang#56986
For golang#59033

Change-Id: I7d03fe34d418aff97a551b236b5d43506e402871
Reviewed-on: https://go-review.googlesource.com/c/go/+/499415
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests