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

Nil pointer dereference panic in Find() if module by prefix is not found #251

Closed
sengleung opened this issue Oct 23, 2023 · 0 comments · Fixed by #252
Closed

Nil pointer dereference panic in Find() if module by prefix is not found #251

sengleung opened this issue Oct 23, 2023 · 0 comments · Fixed by #252

Comments

@sengleung
Copy link
Contributor

Panic stack trace of goyang source files:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x15a7cc0]

goroutine 20 [running]:
panic({0x18493c0, 0x1fe79a0})
        /usr/local/go/src/runtime/panic.go:884 +0x213 fp=0xc003b1ecd8 sp=0xc003b1ec18 pc=0x1036193
runtime.panicmem(...)
        /usr/local/go/src/runtime/panic.go:260
runtime.sigpanic()
        /usr/local/go/src/runtime/signal_unix.go:841 +0x37d fp=0xc003b1ed38 sp=0xc003b1ecd8 pc=0x104db1d
github.com/openconfig/goyang/pkg/yang.(*Module).ParentNode(0x159ea5f?)
        github.com/openconfig/goyang/pkg/yang/yang.go:143 fp=0xc003b1ed40 sp=0xc003b1ed38 pc=0x15a7cc0
github.com/openconfig/goyang/pkg/yang.RootNode({0x1af0840?, 0x0?})
        github.com/openconfig/goyang/pkg/yang/node.go:147 +0x45 fp=0xc003b1ed68 sp=0xc003b1ed40 pc=0x159f0e5
github.com/openconfig/goyang/pkg/yang.module({0x1af0840?, 0x0?})
        github.com/openconfig/goyang/pkg/yang/node.go:159 +0x27 fp=0xc003b1ed98 sp=0xc003b1ed68 pc=0x159f167
github.com/openconfig/goyang/pkg/yang.(*Entry).Find(0xc00660cf00, {0xc001625f80?, 0xc003b1ef40?})
        github.com/openconfig/goyang/pkg/yang/entry.go:1325 +0x425 fp=0xc003b1ee80 sp=0xc003b1ed98 pc=0x1595c85

m := module(FindModuleByPrefix(contextNode, prefix))

  • Find() calls FindModuleByPrefix() which returns nil of type *yang.Module.
  • module() is called with (*yang.Module)(nil).

goyang/pkg/yang/node.go

Lines 158 to 160 in 944052f

func module(n Node) *Module {
m := RootNode(n)
if m.Kind() == "submodule" {

goyang/pkg/yang/node.go

Lines 146 to 147 in 944052f

func RootNode(n Node) *Module {
for ; n.ParentNode() != nil; n = n.ParentNode() {

func (s *Module) ParentNode() Node { return s.Parent }

  • module() calls RootNode() which expects the Node interface type. (*yang.Module)(nil) is passed into RootNode().
  • ParentNode() is called with (*yang.Module)(nil). This is not n == nil because the interface has a nil pointer value (same explanation as this). This results in a nil pointer dereference panic.
  • RootNode() is vulnerable to panics due to a non-nil interface (e.g., when a nil *yang.Module is passed). Even if we properly nil check in RootNode() with reflection, we will again be vulnerable to a panic again with Kind() in node.go:160.

The root cause here is that module() is called with (*yang.Module)(nil). We should nil check FindModuleByPrefix() before calling module() to prevent the panic.

sengleung added a commit to sengleung/goyang that referenced this issue Oct 23, 2023
Previously, if FindModuleByPrefix() returns nil in Find(), it would
continue to call module() with nil, resulting in a nil pointer
dereference panic when trying to call ParentNode() from RootNode().
The fix is to return an error if FindModuleByPrefix() returns nil.

Fixes openconfig#251
wenovus pushed a commit that referenced this issue Oct 26, 2023
Previously, if FindModuleByPrefix() returns nil in Find(), it would
continue to call module() with nil, resulting in a nil pointer
dereference panic when trying to call ParentNode() from RootNode().
The fix is to return an error if FindModuleByPrefix() returns nil.

Fixes #251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant