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

encoding/gob: panic when Encode nil #16204

Closed
tnclong opened this issue Jun 29, 2016 · 4 comments
Closed

encoding/gob: panic when Encode nil #16204

tnclong opened this issue Jun 29, 2016 · 4 comments
Assignees
Milestone

Comments

@tnclong
Copy link
Contributor

@tnclong tnclong commented Jun 29, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.6.2 darwin/amd64
  2. What did you do?
    Use gob to serialize golang object.
    Main Code: https://play.golang.org/p/z0v0O8D_WC
  3. What did you expect to see?
    return error.New("Gob: nil given")
  4. What did you see instead?
    panic error

Thank you very much.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jun 29, 2016

Hello there @JadenTeng, I am not qualified to answer this bug because I don't know much about encoding/gob but I made something here https://play.golang.org/p/8PB1oN2eFO that perhaps could be useful for comparing behaviors across diferent encoders. They all have different behaviors of course, but I found it encoding/gob can successfully encode a nil map, I also think the nil you are passing in being untyped might be a bit of a problem for encoding/gob. I also came across this documentation from encoding/gob
screen shot 2016-06-29 at 2 27 14 am

I don't have the different versions of Go built on this machine to test out when it could have started, but from the source code, it seems like the behavior has been in for a long time https://github.com/golang/go/blame/master/src/encoding/gob/encoder.go so a change might break compatibility with older programs?

  • As for the code to play around with the different encoders json, xml and gob, it is inlined below:
package main

import (
    "bytes"
    "encoding/gob"
    "encoding/json"
    "encoding/xml"
    "fmt"
    "io"
)

type encoder interface {
    Encode(interface{}) error
}

type encoderType int

const (
    JSON encoderType = iota
    XML
    GOB
)

func (e encoderType) String() string {
    switch e {
    case JSON:
        return "json"
    case XML:
        return "xml"
    case GOB:
        return "gob"
    default:
        return "unknown"
    }
}

func (et encoderType) encoderProducer() func(io.Writer) encoder {
    switch et {
    case JSON:
        return func(w io.Writer) encoder { return json.NewEncoder(w) }
    case GOB:
        return func(w io.Writer) encoder { return gob.NewEncoder(w) }
    case XML:
        return func(w io.Writer) encoder { return xml.NewEncoder(w) }
    default:
        return nil
    }
}

func (et encoderType) nilBehavior(src interface{}) (serializedBytes []byte, err interface{}) {
    defer func() {
        if rerr := recover(); rerr != nil {
            switch rerr := rerr.(type) {
            case error:
                err = rerr
            default:
                err = fmt.Errorf("%v", rerr)
            }
        }
    }()
    buf := new(bytes.Buffer)
    encoderFn := et.encoderProducer()
    err = encoderFn(buf).Encode(src)
    serializedBytes = buf.Bytes()
    return
}

func main() {
    var fn func() error = nil
    gob.Register(fn)
    testCases := [...]struct {
        typ       encoderType
        wantErr   error
        wantBytes []byte
        value     interface{}
    }{
        {JSON, nil, []byte("null\n"), nil},
        {JSON, nil, []byte("null\n"), map[string]string(nil)},

        {XML, nil, []byte(""), nil},

        {GOB, nil, []byte(""), nil},
        {GOB, nil, []byte("\x0e\xff\x81\x04\x01\x02\xff\x82\x00\x01\f\x01\f\x00\x00\x04\xff\x82\x00\x00"), map[string]string(nil)},
        {GOB, nil, []byte(""), func() (fn func() error) { return }},
        {GOB, nil, []byte("\x03\f\x00\x00"), new(string)},
    }

    for i, tt := range testCases {
        gotBytes, gotErr := tt.typ.nilBehavior(tt.value)
        if !bytes.Equal(gotBytes, tt.wantBytes) {
            fmt.Printf("#%d: %s: gotBytes=%q wantBytes=%q\n", i, tt.typ, gotBytes, tt.wantBytes)
        }

        if gotErr != tt.wantErr {
            fmt.Printf("#%d: %s: gotErr=%v wantErr=%v\n", i, tt.typ, gotErr, tt.wantErr)
        }
    }
}
@tnclong
Copy link
Contributor Author

@tnclong tnclong commented Jun 29, 2016

so a change might break compatibility with older programs?

I don't think so. Because all Encode(e interface{}) error method return error for programmer to handle.
You always

var buff bytes.Buffer
err := gob.NewEncoder(&buff).Encode(e) 
if err != nil {
        //process err
}

in your program.

There are alternative change should made in the gob:Encode method according to your comment

  1. return err
  2. give []byte{}; return nil // I don't know the []byte should be

I like (2). Because it's same with other package Encode methods behavior.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Jul 5, 2016

@JadenTeng gotcha.
So depending on what gets decided for Go1.8, I think we can catch this check since IMO in the first place we should have been checking to see if .Type() could be invoked on the value passed in. The reflect docs are explicit on the behavior of invalid/zero values panicing when any other methods except .String() are invoked.
https://golang.org/pkg/reflect/#Value
screen shot 2016-07-05 at 12 57 32 am
ie .Type() cannot be invoked on a kind == reflect.Invalid value

In this diff, I catch that check and then panic with an error directly from gob itself instead of letting it propagate to reflect

diff --git a/src/encoding/gob/encoder.go b/src/encoding/gob/encoder.go
index d6c8fdd..14871bc 100644
--- a/src/encoding/gob/encoder.go
+++ b/src/encoding/gob/encoder.go
@@ -215,9 +215,13 @@ func (enc *Encoder) sendTypeId(state *encoderState, ut *userTypeInfo) {
 // guaranteeing that all necessary type information has been transmitted first.
 // Passing a nil pointer to EncodeValue will panic, as they cannot be transmitted by gob.
 func (enc *Encoder) EncodeValue(value reflect.Value) error {
-   if value.Kind() == reflect.Ptr && value.IsNil() {
+   kind := value.Kind()
+   if kind == reflect.Ptr && value.IsNil() {
        panic("gob: cannot encode nil pointer of type " + value.Type().String())
    }
+   if kind == reflect.Invalid {
+       panic("gob: cannot encode an invalid/zero value")
+   }

    // Make sure we're single-threaded through here, so multiple
    // goroutines can share an encoder.

if we decide to instead an error, then we can modify it in this place.

@odeke-em odeke-em self-assigned this Aug 1, 2016
@gopherbot gopherbot closed this in 0a2a64d Aug 19, 2016
@golang golang locked and limited conversation to collaborators Aug 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.