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

PANIC=Format method: runtime error: invalid memory address or nil pointer dereference with go 1.17 #77

Closed
wallrj opened this issue Sep 3, 2021 · 8 comments · Fixed by #87

Comments

@wallrj
Copy link

wallrj commented Sep 3, 2021

When I run the following short script with Go 1.17 it prints a PANIC=Format method message

package main

import (
	"github.com/kr/pretty"
	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func main() {
	o := corev1.Secret{
		ObjectMeta: metav1.ObjectMeta{
			Name: "foo",
		},
	}
	pretty.Println(o)
}
$ go1.16.1 run ./
v1.Secret{
    TypeMeta:   v1.TypeMeta{},
    ObjectMeta: v1.ObjectMeta{
        Name:                       "foo",
        GenerateName:               "",
        Namespace:                  "",
        SelfLink:                   "",
        UID:                        "",
        ResourceVersion:            "",
        Generation:                 0,
        CreationTimestamp:          v1.Time{},
        DeletionTimestamp:          (*v1.Time)(nil),
        DeletionGracePeriodSeconds: (*int64)(nil),
        Labels:                     {},
        Annotations:                {},
        OwnerReferences:            nil,
        Finalizers:                 nil,
        ClusterName:                "",
        ManagedFields:              nil,
    },
    Immutable:  (*bool)(nil),
    Data:       {},
    StringData: {},
    Type:       "",
}
$ go1.17 run ./
v1.Secret{
%!v(PANIC=Format method: runtime error: invalid memory address or nil pointer dereference)
@christophcemper
Copy link

christophcemper commented Mar 11, 2022

Yes,

also getting a panic for time pointers

%!v(PANIC=Format method: value method time.Time.GoString called using nil *Time pointer)

with a *time.Time in a struct that = nil

Go 1.17.6
Prettty v0.3.0

@kr
Copy link
Owner

kr commented Mar 13, 2022

Some relevant info in https://go.dev/issue/20995.

@drinktee
Copy link

Same issue. How to fix ?

@barrettj12
Copy link
Contributor

Still observing this on Go 1.18.5.

@jameinel
Copy link

So the go dev issue 20995 says that the workaround (while a bit ugly) is to essentially check to see if you have a pointer, and whether the Elem of the pointer has the method:

if fieldType.Kind() == reflect.Ptr && field.IsNil() {
	// Method defined on a value receiver.
	if _, ok := fieldType.Elem().MethodByName("Check"); ok {
		continue // Ignore since value can't be retrieved from nil pointer
	}
}

Essentially you are saying "oh, I have a nil pointer, let me check if the method is defined on the Value (non-pointer) of this type".
I'm not sure if that means all of the kr/pretty code that says "you have a custom formatter" then needs to do this check (but the custom formatter is defined on a value type but I have a nil pointer)
It doesn't seem bad to just say "ignore custom formatters if you have a nil pointer"

benhoyt added a commit to canonical/pretty that referenced this issue Aug 18, 2022
This handles a few cases (similar to how fmt %#v does):

1. A GoString method on a value receiver, called with a nil pointer
2. A GoString method on a pointer receiver that doesn't check for nil
3. A GoString method that panics in some other way

Because Go 1.17 added a method Time.GoString with value receiver, this
broke structs that had *time.Time fields with nil values (which is
common!).

Also added a bunch of tests for these cases.

Fixes kr#77
benhoyt added a commit to canonical/pretty that referenced this issue Aug 21, 2022
This handles a few cases (similar to how fmt %#v does):

- A GoString method on a value receiver, called with a nil pointer
- A GoString method on a pointer receiver that doesn't check for nil
- A GoString method that panics in some other way

Because Go 1.17 added a method Time.GoString with value receiver, this
broke structs that had *time.Time fields with nil values (which is
common!).

Also added a bunch of tests for these cases.

This should address kr#77

Co-authored-by: Jordan Barrett <jordan.barrett@canonical.com>
@benhoyt
Copy link
Contributor

benhoyt commented Aug 21, 2022

We fixed this in our canonical fork at canonical@771f4dc (see discussion at canonical#1). It's fixed in a similar way to how fmt does when calling GoString and other user-defined methods. I've also created a pull request here with the same fix: #87

jujubot added a commit to juju/juju that referenced this issue Aug 22, 2022
#14493

This pulls through a fix for issue kr/pretty#77 which was causing some of our logs to format like this:
```
[LOG] 0:00.001 DEBUG juju.kubernetes.provider.application creating storage class for gitlab filesystem database: &v1.StorageClass{
%!v(PANIC=Format method: runtime error: invalid memory address or nil pointer dereference)
[LOG] 0:00.001 DEBUG juju.kubernetes.provider.application using persistent volume claim for gitlab filesystem database: v1.PersistentVolumeClaim{
%!v(PANIC=Format method: runtime error: invalid memory address or nil pointer dereference)
```

We've pushed the patch upstream, but [kr/pretty](https://github.com/kr/pretty) doesn't look actively maintained anymore. Until they accept our patch, we will use our fork at [canonical/pretty](https://github.com/canonical/pretty) instead.

## QA steps

```sh
go test -v ./caas/kubernetes/provider/application -check.f applicationSuite.TestEnsureStateful -check.vv
```
Verify that the structs are printed correctly in the logs.
@kr kr closed this as completed in #87 Aug 25, 2022
kr pushed a commit that referenced this issue Aug 25, 2022
This handles a few cases (similar to how fmt %#v does):

- A GoString method on a value receiver, called with a nil pointer
- A GoString method on a pointer receiver that doesn't check for nil
- A GoString method that panics in some other way

Because Go 1.17 added a method Time.GoString with value receiver, this
broke structs that had *time.Time fields with nil values (which is
common!).

Also added a bunch of tests for these cases.

Fixes #77

Co-authored-by: Jordan Barrett <jordan.barrett@canonical.com>
@PrMinisterGR
Copy link

Shouldn't there be a new proper release with this fix integrated?

@kr
Copy link
Owner

kr commented Oct 7, 2022

Shouldn't there be a new proper release with this fix integrated?

Yes indeed. Just tagged v0.3.1. Thank you.

tjex added a commit to zk-org/pretty that referenced this issue Jan 10, 2024
* Update Dependencies  (kr#75)

This updates github.com/rogpeppe/go-internal from 1.6.1 to 1.8.0
and the github action 'setup-go' from v1 to v2.

It also adds a config file for dependabot to automatically check
this repo for new dependency versions weekly.

* Bump actions/checkout from 2 to 3 (kr#84)

Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 3.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v2...v3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Catch panics when calling GoString like fmt %#v does (kr#87)

This handles a few cases (similar to how fmt %#v does):

- A GoString method on a value receiver, called with a nil pointer
- A GoString method on a pointer receiver that doesn't check for nil
- A GoString method that panics in some other way

Because Go 1.17 added a method Time.GoString with value receiver, this
broke structs that had *time.Time fields with nil values (which is
common!).

Also added a bunch of tests for these cases.

Fixes kr#77

Co-authored-by: Jordan Barrett <jordan.barrett@canonical.com>

* Bump github.com/rogpeppe/go-internal from 1.8.0 to 1.9.0 (kr#88)

This change bumps github.com/rogpeppe/go-internal from version 1.8.0 to 1.9.0.

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix failing test

* bump go version to same as zk, and update workflow

* update go.sum

* update go.sum - again

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Matthieu MOREL <mmorel-35@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
Co-authored-by: Jordan Barrett <jordan.barrett@canonical.com>
Co-authored-by: tjex <tjex@tjex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants