Unmarshal plist with duplicate keys results in panic

[hilljgo]

When attempting to unmarshal a plist that contains duplicate keys, a panic is hit.

Example code:

package main

import (
    "fmt"

    plist "github.com/DHowett/go-plist"
)

var duplicateKeyPlist = `
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>PayloadIdentifier</key>
    <string>foo</string>
    <key>PayloadIdentifier</key>
    <string>bar</string>
</dict>
</plist>
`

func main() {

    unmarshaledPlist := make(map[string]interface{})
    _, err := plist.Unmarshal([]byte(duplicateKeyPlist), &unmarshaledPlist)
    if err != nil {
        fmt.Printf("Error: %+v\n", err)
        return
    }

    return
}

The following panic stack trace is returned:

panic: reflect: reflect.Value.Set using unaddressable value [recovered]
    panic: interface conversion: string is not error: missing method Error

goroutine 1 [running]:
plistbug/vendor/github.com/DHowett/go-plist.(*Decoder).Decode.func1(0xc420045ef0)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/decode.go:32 +0x8f
panic(0x10db060, 0xc42000e3a0)
    /usr/local/Cellar/go/1.10.1/libexec/src/runtime/panic.go:502 +0x229
reflect.flag.mustBeAssignable(0x94)
    /usr/local/Cellar/go/1.10.1/libexec/src/reflect/value.go:234 +0x15c
reflect.Value.Set(0x10e2760, 0xc42000e380, 0x94, 0x10db060, 0xc42000e390, 0x98)
    /usr/local/Cellar/go/1.10.1/libexec/src/reflect/value.go:1367 +0x2f
plistbug/vendor/github.com/DHowett/go-plist.(*Decoder).unmarshal(0xc42000a0c0, 0x111ad80, 0xc42000e330, 0x10e2760, 0xc42000e380, 0x94)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/unmarshal.go:107 +0xbc9
plistbug/vendor/github.com/DHowett/go-plist.(*Decoder).unmarshalDictionary(0xc42000a0c0, 0xc42007a390, 0x10e4660, 0xc42000c028, 0x195)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/unmarshal.go:265 +0x127
plistbug/vendor/github.com/DHowett/go-plist.(*Decoder).unmarshal(0xc42000a0c0, 0x111ab00, 0xc42007a390, 0x10d5cc0, 0xc42000c028, 0x16)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/unmarshal.go:196 +0x8a9
plistbug/vendor/github.com/DHowett/go-plist.(*Decoder).Decode(0xc42000a0c0, 0x10d5cc0, 0xc42000c028, 0x0, 0x0)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/decode.go:76 +0x27f
plistbug/vendor/github.com/DHowett/go-plist.Unmarshal(0xc42009c000, 0x125, 0x140, 0x10d5cc0, 0xc42000c028, 0x140, 0x0, 0x0)
    /Users/<myname>/Go/src/plistbug/vendor/github.com/DHowett/go-plist/decode.go:116 +0xd0
main.main()
    /Users/<myname>/Go/src/plistbug/main.go:25 +0xb7
exit status 2

[DHowett]

Awesome, thanks! I wonder what the correct behavior is here. It's clearly not panicking. However, it can't produce a useful, safe, two-way serializable answer in every case.

[DHowett]

(Also: no property list emitter should have generated one with duplicate keys. Is it handcrafted?)

[hryx]

Hi @DHowett. I worked with @hilljgo when we uncovered this, and yep, it was exposed by a hand-crafted plist, which is a common use-case for us.

As I see it, there are two ways to go:

1. Treat redundant dictionary keys forgivingly, and let the last encountered value for a given key overwrite any previous ones for the same key. From what I've seen, some deserializers have this behavior (even if it isn't specified or guaranteed). In the example provided in the description above, the final value of PayloadIdentifier would be bar.
2. Return an error. The signature of Decode() already seems to have this intention, but this type assertion inside a recover is failing because panic was called somewhere with a string instead of an error.

Both seem like fine approaches to me, with number 2 likely being a quicker fix and more explicit behavior. Do you have a preference?

[DHowett]

1. Apple's property list parsers take the first entry with a given key to be canonical.
2. This only happens when you unmarshal into a map[string]interface{}, not an interface{} (!), and
3. I chose to fix it with approach 1. encoding/json does the same thing, and 2 ended up being somewhat more burdensome.

[DHowett]

Ah, approach 3 (act like Apple) is the one that proved more burdensome. I didn't investigate 2, but I did determine that package reflect is incorrectly panicking with a string.

(If I switch to package cf (separate branch on this repo), which uses true maps to back cf.Dictionary, the balance will shift to taking Apple's approach and not letting the duplicate value escape the parser.)