mvdan / fdroidcl

F-Droid desktop client
BSD 3-Clause "New" or "Revised" License
269 stars 22 forks source link

"panic: runtime error: invalid memory address or nil pointer dereference" on app install #19

Closed relan closed 8 years ago

relan commented 8 years ago

When apps []fdroidcl.App are loaded from cache-gob they have nil repo pointer. This causes a crash on each install after the first one (when data was loaded from XML):

$ fdroidcl install com.vonglasow.michael.satstat
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x4aba19]

goroutine 1 [running]:
panic(0x8147a0, 0xc82000e150)
    /usr/lib/golang/src/runtime/panic.go:481 +0x3e6
github.com/mvdan/fdroidcl.(*Apk).URL(0xc8205ee700, 0x0, 0x0)
    /home/relan/opt/golang/src/github.com/mvdan/fdroidcl/index.go:203 +0x1c9
main.downloadApk(0xc8205ee700, 0x0, 0x0)
    /home/relan/opt/golang/src/github.com/mvdan/fdroidcl/cmd/fdroidcl/download.go:40 +0x42
main.downloadAndDo(0xc82002a000, 0x1, 0x1, 0xc8200ca770, 0x95b458)
    /home/relan/opt/golang/src/github.com/mvdan/fdroidcl/cmd/fdroidcl/install.go:49 +0x1f4
main.runInstall(0xc82000a3e0, 0x1, 0x1)
    /home/relan/opt/golang/src/github.com/mvdan/fdroidcl/cmd/fdroidcl/install.go:35 +0x2a7
main.main()
    /home/relan/opt/golang/src/github.com/mvdan/fdroidcl/cmd/fdroidcl/main.go:180 +0x3fb

I use golang-bin-1.6.3-2.fc24.x86_64.

This patch fixes the issue but breaks backward compatibility because it changes cache-gob format:

diff --git a/index.go b/index.go
index fcce635..31c1e77 100644
--- a/index.go
+++ b/index.go
@@ -86,7 +86,7 @@ func (a *App) IconURLForDensity(density IconDensity) string {
    if len(a.Apks) == 0 {
        return ""
    }
-   return fmt.Sprintf("%s/%s/%s", a.Apks[0].repo.URL,
+   return fmt.Sprintf("%s/%s/%s", a.Apks[0].RepoURL,
        getIconsDir(density), a.Icon)
 }

@@ -196,15 +196,15 @@ type Apk struct {
    Hash    HexHash   `xml:"hash"`

    AppID string `xml:"-"`
-   repo  *Repo  `xml:"-"`
+   RepoURL string `xml:"-"`
 }

 func (a *Apk) URL() string {
-   return fmt.Sprintf("%s/%s", a.repo.URL, a.ApkName)
+   return fmt.Sprintf("%s/%s", a.RepoURL, a.ApkName)
 }

 func (a *Apk) SrcURL() string {
-   return fmt.Sprintf("%s/%s", a.repo.URL, a.SrcName)
+   return fmt.Sprintf("%s/%s", a.RepoURL, a.SrcName)
 }

 func (a *Apk) IsCompatibleABI(ABIs []string) bool {
@@ -260,7 +260,7 @@ func LoadIndexXML(r io.Reader) (*Index, error) {
        for j := range app.Apks {
            apk := &app.Apks[j]
            apk.AppID = app.ID
-           apk.repo = &index.Repo
+           apk.RepoURL = index.Repo.URL
        }
    }
    return &index, nil

Users need to delete ~/.config/fdroidcl/cache-gob after applying this patch.

mvdan commented 8 years ago

Wow this is pretty bad. Thanks for reporting! Will do a 0.3.1.

relan commented 8 years ago

I was a bit too fast to conclude that pointers aren't saved into cache-gob. My patch didn't actually solve the issue and after f48dc32 I get this:

$ fdroidcl install me.writeily
Downloading /me.writeily_8.apk... 
2016/09/28 09:46:25 Could not download me.writeily: Get /me.writeily_8.apk: unsupported protocol scheme ""

Looks like repoURL isn't saved into cache-gob at all. This command shows nothing:

$ strings ~/.cache/fdroidcl/cache-gob | grep -F f-droid.org/repo/
mvdan commented 8 years ago

@relan care to share your config file?

relan commented 8 years ago

Here it is:

{
    "repos": [
        {
            "id": "f-droid",
            "url": "https://f-droid.org/repo",
            "enabled": true
        },
        {
            "id": "f-droid-archive",
            "url": "https://f-droid.org/archive",
            "enabled": false
        },
        {
            "id": "binary",
            "url": "http://localhost/fdroid/repo",
            "enabled": true
        }
    ]
}
mvdan commented 8 years ago

FWIW the strings line results in no output here either.

Then again, I'm hitting the problem now. What the hell.

mvdan commented 8 years ago

My bad, I wasn't exporting RepoURL which was causing encoding/gob to not store the value.

This was my mistake, as your patch did indeed export the field.