[GH-ISSUE #69] Panic on deletion of broken zonefile

kerem commented

2026-02-28 14:16:07 +03:00

Owner

Originally created by @abligh on GitHub (Aug 15, 2015).
Original GitHub issue: https://github.com/abh/geodns/issues/69

geodns panics if a broken zone file is created then deleted.

To replicate, create a broken zonefile:

$ echo a > dns/x.com.json

Wait until the zone file fails to load:

geodns 13:25:19.160763 log.go:7: Reading new file x.com.json
geodns 13:25:19.160889 zones.go:69: Caught an error error parsing JSON object in config file dns/x.com.json:
Error at line 1, column 1 (file offset 1):
    1: a
      ^
invalid character 'a' looking for beginning of value

Then delete the zone file

$ rm dns/x.com.json

And wait a few minutes

geodns 13:25:29.163572 zones.go:91: Removing zone
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x400fe45]

goroutine 29 [running]:
main.(*Zone).Close(0xc20805e6e0)
    /Users/amb/go/src/github.com/abh/geodns/zone.go:98 +0x1a5
main.zonesReadDir(0x4489e70, 0x6, 0xc20803de30, 0x0, 0x0)
    /Users/amb/go/src/github.com/abh/geodns/zones.go:92 +0x78b
main.zonesReader(0x4489e70, 0x6, 0xc20803de30)
    /Users/amb/go/src/github.com/abh/geodns/zones.go:26 +0x3c
created by main.main
    /Users/amb/go/src/github.com/abh/geodns/geodns.go:173 +0xd41
 ... (snipped) ...

The problem is this line:

func (z *Zone) Close() {
    metrics.Unregister(z.Origin + " queries")
    metrics.Unregister(z.Origin + " EDNS queries")
    z.Metrics.LabelStats.Close() // <-------------------- HERE
    z.Metrics.ClientStats.Close()
}

As the zone file never existed, it has been incompletely loaded, and the metrics were not set up. Deleting it attempts to Close() the zone file and this fails with a nil pointer dereference as z.Metrics is nil.

I would suggest zone files that are not present prior to loading and fail to load leave the zones map unchanged and do nothing. I will submit a changeset to fix this.

Originally created by @abligh on GitHub (Aug 15, 2015). Original GitHub issue: https://github.com/abh/geodns/issues/69 geodns panics if a broken zone file is created then deleted. To replicate, create a broken zonefile: ``` $ echo a > dns/x.com.json ``` Wait until the zone file fails to load: ``` geodns 13:25:19.160763 log.go:7: Reading new file x.com.json geodns 13:25:19.160889 zones.go:69: Caught an error error parsing JSON object in config file dns/x.com.json: Error at line 1, column 1 (file offset 1): 1: a ^ invalid character 'a' looking for beginning of value ``` Then delete the zone file ``` $ rm dns/x.com.json ``` And wait a few minutes ``` geodns 13:25:29.163572 zones.go:91: Removing zone panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x0 pc=0x400fe45] goroutine 29 [running]: main.(*Zone).Close(0xc20805e6e0) /Users/amb/go/src/github.com/abh/geodns/zone.go:98 +0x1a5 main.zonesReadDir(0x4489e70, 0x6, 0xc20803de30, 0x0, 0x0) /Users/amb/go/src/github.com/abh/geodns/zones.go:92 +0x78b main.zonesReader(0x4489e70, 0x6, 0xc20803de30) /Users/amb/go/src/github.com/abh/geodns/zones.go:26 +0x3c created by main.main /Users/amb/go/src/github.com/abh/geodns/geodns.go:173 +0xd41 ... (snipped) ... ``` The problem is this line: ``` func (z *Zone) Close() { metrics.Unregister(z.Origin + " queries") metrics.Unregister(z.Origin + " EDNS queries") z.Metrics.LabelStats.Close() // <-------------------- HERE z.Metrics.ClientStats.Close() } ``` As the zone file never existed, it has been incompletely loaded, and the metrics were not set up. Deleting it attempts to `Close()` the zone file and this fails with a `nil` pointer dereference as `z.Metrics` is `nil`. I would suggest zone files that are not present prior to loading and fail to load leave the zones map unchanged and do nothing. I will submit a changeset to fix this.

kerem closed this issue

2026-02-28 14:16:07 +03:00

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abh commented on GitHub (Aug 15, 2015):

Woah, that's dumb. My zone files are almost always built by other tools so I haven't run into this. :-) Thank you. I'll have a look at your fix.

@abh commented on GitHub (Aug 15, 2015): Woah, that's dumb. My zone files are almost always built by other tools so I haven't run into this. :-) Thank you. I'll have a look at your fix.

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abh commented on GitHub (Aug 21, 2015):

Hi Alex, I tried fixing it a little differently by not setting up the half-broken zone config when the file is invalid. I can't figure out why I was doing that in the first place, it just seems like it defeats the purpose. The intention was that if you screw up the zone data, the last loaded zone would remain active. I think it does that now (though I didn't explicitly test it or look carefully; it's late here now).

@abh commented on GitHub (Aug 21, 2015): Hi Alex, I tried fixing it a little differently by not setting up the half-broken zone config when the file is invalid. I can't figure out why I was doing that in the first place, it just seems like it defeats the purpose. The intention was that if you screw up the zone data, the last loaded zone would remain active. I think it does that now (though I didn't explicitly test it or look carefully; it's late here now).

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 21, 2015):

I think the reason you may have done it the other way is to avoid it trying to load broken zone files every 5 seconds. Unless there is a zone stored, there is no modtime stored, and on each poll it thinks it's a new file.

@abligh commented on GitHub (Aug 21, 2015): I think the reason you may have done it the other way is to avoid it trying to load broken zone files every 5 seconds. Unless there is a zone stored, there is no `modtime` stored, and on each poll it thinks it's a new file.

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 21, 2015):

Oh, and I think the other issue may be that you want to keep the stats of zone files that are temporarily broken. Else they get cleared out and recreated across the file being broken and recreated.

@abligh commented on GitHub (Aug 21, 2015): Oh, and I think the other issue may be that you want to keep the stats of zone files that are temporarily broken. Else they get cleared out and recreated across the file being broken and recreated.

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abh commented on GitHub (Aug 21, 2015):

Hmn, at a glance i don't think this clears out the stats as it leaves the old zones in place. The reloading every five seconds is a thing, yes. Hmn. Not sure if it matters, does it?

It's probably easy to fix for existing zones that gets mangled.

There's a race condition on line 34 anyway; it might be time to give this code a good overhaul. :-)

@abh commented on GitHub (Aug 21, 2015): Hmn, at a glance i don't think this clears out the stats as it leaves the old zones in place. The reloading every five seconds is a thing, yes. Hmn. Not sure if it matters, does it? It's probably easy to fix for existing zones that gets mangled. There's a race condition on line 34 anyway; it might be time to give this code a good overhaul. :-)

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 21, 2015):

I suspect the fix might be to take the mod time out of the Zone structure and only load zones into the slice when they are valid. I might take a look at that. I was thinking about putting fsnotify in too (though in the end decided it was overkill)

@abligh commented on GitHub (Aug 21, 2015): I suspect the fix might be to take the mod time out of the Zone structure and only load zones into the slice when they are valid. I might take a look at that. I was thinking about putting fsnotify in too (though in the end decided it was overkill)

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 21, 2015):

I'm missing the race condition. Line 34 is:

zones[name] = config

The only thing that should be using zones is the thread that rereads the configs. The handlers themselves are passed the config struct after this. What am I missing?

@abligh commented on GitHub (Aug 21, 2015): I'm missing the race condition. Line 34 is: ``` zones[name] = config ``` The only thing that should be using `zones` is the thread that rereads the configs. The handlers themselves are passed the `config` struct after this. What am I missing?

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abh commented on GitHub (Aug 21, 2015):

Re your fix for the mod time: yes, please. That's what I'd been thinking, too. I had the same thought with fsnotify. Appealing, but too complicated.

The race is with the code in stathat.go (line ~25). I'm planning to just take the stathat stuff out and replace it either with something that talks to InfluxDB or (ideally) something more generic that just makes stats. My stats needs are a little weird because of the NTP Pool.

@abh commented on GitHub (Aug 21, 2015): Re your fix for the mod time: yes, please. That's what I'd been thinking, too. I had the same thought with fsnotify. Appealing, but too complicated. The race is with the code in stathat.go (line ~25). I'm planning to just take the stathat stuff out and replace it either with something that talks to InfluxDB or (ideally) something more generic that just makes stats. My stats needs are a little weird because of the NTP Pool.

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 31, 2015):

Fix to mod time in pull request #77

@abligh commented on GitHub (Aug 31, 2015): Fix to mod time in pull request #77

kerem commented

2026-02-28 14:16:08 +03:00

Author

Owner

@abligh commented on GitHub (Aug 31, 2015):

Re the StatHat thing, I suspect we need something that will iterate Zones under a mutex or some other form of lock protection - probably not just StatHat that needs this. Perhaps a

func (Zones * zones) forEach( ... )

or similar.

@abligh commented on GitHub (Aug 31, 2015): Re the StatHat thing, I suspect we need something that will iterate `Zones` under a mutex or some other form of lock protection - probably not just StatHat that needs this. Perhaps a ``` func (Zones * zones) forEach( ... ) ``` or similar.

kerem referenced this issue

2026-03-01 17:24:26 +03:00

[GH-ISSUE #55] Add tests for targeting by IPv6 address #187

kerem referenced this issue

2026-03-13 14:40:37 +03:00

[GH-ISSUE #55] Add tests for targeting by IPv6 address #329

Rows
Columns

[GH-ISSUE #69] Panic on deletion of broken zonefile #55