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 · 69 comments
Closed

extended backwards compatibility for Go #56986

rsc opened this issue Nov 29, 2022 · 69 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
Member

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
Member

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.

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

Change https://go.dev/cl/500956 mentions this issue: doc/go1.21: document forward and backward compatibility

gopherbot pushed a commit that referenced this issue Jun 5, 2023
Also handle go test -c TODO.

For #15513.
For #56986.
For #57001.

Change-Id: I571ae25d8d8fcd44cb38ac16cdd2a1180016eb94
Reviewed-on: https://go-review.googlesource.com/c/go/+/500956
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: David Chase <drchase@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 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 golang#56986.

Fixes golang#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>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 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 golang#56986.

Fixes golang#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>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 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 golang#56986.

Fixes golang#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>
@kleinkk76

This comment was marked as spam.

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