bgp / stayrtr

RPKI-To-Router server implementation in Go
BSD 3-Clause "New" or "Revised" License
91 stars 13 forks source link

Crash when cache URL isn't present #39

Closed randomthingsandstuff closed 2 years ago

randomthingsandstuff commented 2 years ago
./dist/stayrtr-0.1-62-gcc539c4-linux-x86_64 -metrics.addr "127.0.0.1:9847" -cache http://127.0.0.1:8081/output.json
ERRO[0000] Error updating: HTTP 503 Service Unavailable 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x7f7c4e]

goroutine 1 [running]:
main.(*state).updateFromNewState(0xc0000c6000)
        ./stayrtr.go:258 +0x4e
main.run()
        ./stayrtr.go:595 +0xaf5
main.main()
        ./stayrtr.go:488 +0x19
randomthingsandstuff commented 2 years ago

updateFromNewState() gets called unconditionally after first hit by updateFile() during startup. If updateFile() errors out, s.lastdata doesn't get populated. This nil pointer gets dereferenced and then crashes. This does not appear affect already running scenario because s.lastdata would have been populated or crashed on first run and error checking is in place that ensures the pointer is not changed if there is an error retrieving or deserializing.

Frame 4: ./stayrtr.go:258 (PC: a242a4)
   253: // Update the state based on the current slurm file and data.
   254: func (s *state) updateFromNewState() error {
   255:         sessid, _ := s.server.GetSessionId(nil)
   256:
   257:         if s.checktime {
=> 258:                 buildtime, err := time.Parse(time.RFC3339, s.lastdata.Metadata.Buildtime)
   259:                 if err != nil {
   260:                         return err
   261:                 }
   262:                 notafter := buildtime.Add(time.Hour * 24)
   263:                 if time.Now().UTC().After(notafter) {
(dlv) print s.lastdata
*github.com/bgp/stayrtr/prefixfile.VRPList nil

Fundamentally, this defect can be fixed either by:

1) Initializing a placeholder (empty?) structure or 2) explicitly handling nil pointer scenario and ensure that other uninitialized components (s.lasthash, s.lastchange) are also not problematic

In case # 1, the empty structure could be an empty VRPList. On initialization of a server object, an empty VRPList is created and pointed to via lastdata. Because updatefile() relies on the hash provided in lasthash and the comparison function is bytes.Compare, the empty []byte slice will never compare as equal (until later populated). This results in routineUpdate() retrying based on its existing timers.

In case # 2, updateFromNewState() would check for nil s.lastdata pointer and return an error. This would leave the pointer alone until updateFile() finally gets the file correctly. Other places in code would need to be aware of the possibility of nil-ity to avoid future problems.

To avoid changing more than required and reduce silliness of forgetting nil pointers, I'll use # 1 by instantiating an empty VRPList in the state.lastdata member. It is probably possible to simply change the lastdata to not be a pointer to resolve this issue, but it requires more reading and thinking to be sure of side effects.

randomthingsandstuff commented 2 years ago

Issue was fixed, but also see now that the refresh time is really too long to wait. By default, it is 1 hour. While it will probably work in an hour, this should probably complete a bit faster.

Since s.lastchange.IsZero() will only be true prior to fetch, check this value and set the refresh to 30 seconds when true.