Panic on Browse with nil NodeID

[henripqt]

ISSUE:

When attempting to trigger a client.Browse call I'm getting a panic from the ua.NodeID.Encode method in cases where NodeID == nil

STEPS TO REPRODUCE:

• Install & start OPCUA Simulation Server
• Run the following

package main

import (
	"context"

	"log"

	"github.com/gopcua/opcua"
	"github.com/gopcua/opcua/ua"
)

func main() {
	c, err := newClient(context.Background())
	if err != nil {
		panic(err)
	}

	defer c.Close()

	nsIDs := []string{"ns=3;s=Random_cfg", "ns=3;i=1003", "ns=3;i=1004", "ns=3;i=1005"}
	var browserDescs []*ua.BrowseDescription

	for _, nodeID := range nsIDs {
		nID, err := ua.ParseNodeID(nodeID)
		if err != nil {
			log.Println(err)
			continue
		}

		browserDescs = append(browserDescs, &ua.BrowseDescription{
			NodeID:          nID,
			BrowseDirection: ua.BrowseDirection(ua.BrowseDirectionBoth),
			IncludeSubtypes: false,
			NodeClassMask:   1,
			ResultMask:      1,
		})
	}

	_, err = c.Browse(&ua.BrowseRequest{NodesToBrowse: browserDescs})

	if err != nil {
		log.Println(err)
	}

	select {}
}

// newClient creates a new client and connects to the endpoint.
func newClient(ctx context.Context) (*opcua.Client, error) {
	client := opcua.NewClient("opc.tcp://127.0.0.1:4840/OPCUA/SimulationServer")

	if err := client.Connect(ctx); err != nil {
		return nil, err
	}

	return client, nil
}

The program will panic at the following line :

func (n *NodeID) Encode() ([]byte, error) {
	buf := NewBuffer(nil)
	buf.WriteByte(byte(n.mask)) // <---- Panic here. n == nil
// [...]

If I look over the ExtensionObject.Encode method we see that we have a safeguard for such cases

func (e *ExtensionObject) Encode() ([]byte, error) {
	buf := NewBuffer(nil)
	if e == nil { // <---- Safeguard
		e = &ExtensionObject{TypeID: NewTwoByteExpandedNodeID(0), EncodingMask: ExtensionObjectEmpty}
	}
// [...]

We should implement such safety over the NodeID.Encode method in order to prevent the program from panicking and/or avoid attempting to encode a nil NodeID

[magiconair]

I can reproduce this:

rank@better ~/s/g/g/opcua (main)> go run ./examples/issue-506/main.go 
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x1029bd11c]

goroutine 1 [running]:
github.com/gopcua/opcua/ua.(*NodeID).Encode(0x0)
        /Users/frank/src/github.com/gopcua/opcua/ua/node_id.go:409 +0x2c
github.com/gopcua/opcua/ua.encode({0x102b666e0, 0x140001fc6d0, 0x196}, {0x140002bf840, 0x32})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:46 +0xa98
github.com/gopcua/opcua/ua.writeStruct({0x102b5f5c0, 0x140001fc6c0, 0x199}, {0x140000170b0, 0x22})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:99 +0x1a0
github.com/gopcua/opcua/ua.encode({0x102b5f5c0, 0x140001fc6c0, 0x199}, {0x140000170b0, 0x22})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:83 +0x904
github.com/gopcua/opcua/ua.encode({0x102b28c60, 0x14000029620, 0x196}, {0x140000170b0, 0x22})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:81 +0x758
github.com/gopcua/opcua/ua.writeSlice({0x102b2e660, 0x140001fc828, 0x197}, {0x140000261a0, 0x1f})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:131 +0x590
github.com/gopcua/opcua/ua.encode({0x102b2e660, 0x140001fc828, 0x197}, {0x140000261a0, 0x1f})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:85 +0x7dc
github.com/gopcua/opcua/ua.writeStruct({0x102b58960, 0x140001fc810, 0x199}, {0x102b202ae, 0x11})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:99 +0x1a0
github.com/gopcua/opcua/ua.encode({0x102b58960, 0x140001fc810, 0x199}, {0x102b202ae, 0x11})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:83 +0x904
github.com/gopcua/opcua/ua.encode({0x102b3db60, 0x140001fc810, 0x16}, {0x102b202ae, 0x11})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:81 +0x758
github.com/gopcua/opcua/ua.Encode({0x102b3db60, 0x140001fc810})
        /Users/frank/src/github.com/gopcua/opcua/ua/encode.go:34 +0x134
github.com/gopcua/opcua/ua.(*Buffer).WriteStruct(0x140002d9a48, {0x102b3db60, 0x140001fc810})
        /Users/frank/src/github.com/gopcua/opcua/ua/buffer.go:306 +0x70
github.com/gopcua/opcua/uasc.(*Message).EncodeChunks(0x14000060e60, 0x1feb)
        /Users/frank/src/github.com/gopcua/opcua/uasc/message.go:126 +0x74
github.com/gopcua/opcua/uasc.(*SecureChannel).sendAsyncWithTimeout(0x14000103450, {0x102b766c8, 0x140001fc810}, 0x4, 0x1400014e5b0, 0x140001fc090, 0x1, 0x2540be400)
        /Users/frank/src/github.com/gopcua/opcua/uasc/secure_channel.go:775 +0x3b8
github.com/gopcua/opcua/uasc.(*SecureChannel).sendRequestWithTimeout(0x14000103450, {0x102b766c8, 0x140001fc810}, 0x4, 0x1400014e5b0, 0x140001fc090, 0x2540be400, 0x140002d9e38)
        /Users/frank/src/github.com/gopcua/opcua/uasc/secure_channel.go:674 +0xa0
github.com/gopcua/opcua/uasc.(*SecureChannel).SendRequestWithTimeout(0x14000103450, {0x102b766c8, 0x140001fc810}, 0x140001fc090, 0x2540be400, 0x14000109e38)
        /Users/frank/src/github.com/gopcua/opcua/uasc/secure_channel.go:738 +0xb4
github.com/gopcua/opcua.(*Client).sendWithTimeout(0x14000103380, {0x102b766c8, 0x140001fc810}, 0x2540be400, 0x14000109e38)
        /Users/frank/src/github.com/gopcua/opcua/client.go:766 +0xb8
github.com/gopcua/opcua.(*Client).Send(...)
        /Users/frank/src/github.com/gopcua/opcua/client.go:752
github.com/gopcua/opcua.(*Client).Browse(0x14000103380, 0x140001fc810)
        /Users/frank/src/github.com/gopcua/opcua/client.go:849 +0x80
main.main()
        /Users/frank/src/github.com/gopcua/opcua/examples/issue-506/main.go:39 +0x30c
exit status 2

[magiconair]

This is not doing what you think it does:

browserDescs := make([]*ua.BrowseDescription, len(nsIDs))

It should be

browserDescs := make([]*ua.BrowseDescription, 0, len(nsIDs))

or even better just

var browserDescs []*ua.BrowseDescription

Keep it simple. Go manages the memory for you efficiently. Code still panics though.

[henripqt]

@magiconair Thanks for pointing it out. Will update the ticket

[magiconair]

This is also problematic since the inner nodeID of type *ua.NodeID shadows the outer nodeID loop variable of type string. The compiler allows it but you shouldn't write code like this.

	for _, nodeID := range nsIDs {
		nodeID, err := ua.ParseNodeID(nodeID)

[magiconair]

So the main problem is that your BrowseRequest is incomplete which triggers the nil pointer exception. Have a look at the References method for the Node (https://github.com/gopcua/opcua/blob/main/node.go#L167-L197) but I agree that this isn't obvious.

You have to provide a View in the request with a non-empty ViewID and also a ReferenceTypeID in the BrowseDescription. NodeIDs cannot be nil since that concept doesn't exist in OPC/UA. You then have to provide a 0 node id instead.

I was struggling with this when I wrote the encoder on whether we should make the NodeID smart enough to encode nil as ua.NewTwoByteNodeID(0).

@kung-foo do you have an opinion on this?

[magiconair]

This works:

package main

import (
	"context"
	"log"

	"github.com/gopcua/opcua"
	"github.com/gopcua/opcua/id"
	"github.com/gopcua/opcua/ua"
)

func main() {
	c, err := newClient(context.Background())
	if err != nil {
		panic(err)
	}
	defer c.Close()

	nodeIDs := []string{"ns=3;s=Random_cfg", "ns=3;i=1003", "ns=3;i=1004", "ns=3;i=1005"}
	var browserDescs []*ua.BrowseDescription
	for _, nodeID := range nodeIDs {
		nid, err := ua.ParseNodeID(nodeID)
		if err != nil {
			log.Println(err)
			continue
		}

		browserDescs = append(browserDescs, &ua.BrowseDescription{
			NodeID:          nid,
			ReferenceTypeID: ua.NewNumericNodeID(0, id.References),
			BrowseDirection: ua.BrowseDirection(ua.BrowseDirectionBoth),
			IncludeSubtypes: false,
			NodeClassMask:   1,
			ResultMask:      1,
		})
	}

	_, err = c.Browse(&ua.BrowseRequest{
		View: &ua.ViewDescription{
			ViewID: ua.NewTwoByteNodeID(0),
		},
		NodesToBrowse: browserDescs,
	})

	if err != nil {
		log.Println(err)
	}

	select {}
}

// newClient creates a new client and connects to the endpoint.
func newClient(ctx context.Context) (*opcua.Client, error) {
	client := opcua.NewClient("opc.tcp://127.0.0.1:4840/OPCUA/SimulationServer")

	if err := client.Connect(ctx); err != nil {
		return nil, err
	}

	return client, nil
}

[magiconair]

This would fix the behaviour. You still have to provide an empty &ua.ViewDescrption{} and a ReferenceTypeID for the browse request to work. So I'm not sure whether it is worth it. But I'll create a PR and then we can have the discussion there.

diff --git a/ua/node_id.go b/ua/node_id.go
index 721845c..6585ece 100644
--- a/ua/node_id.go
+++ b/ua/node_id.go
@@ -405,6 +405,12 @@ func (n *NodeID) Decode(b []byte) (int, error) {
 }
 
 func (n *NodeID) Encode() ([]byte, error) {
+       // https://github.com/gopcua/opcua/issues/506
+       // encode 'nil' node ids as two byte zero values
+       if n == nil {
+               return []byte{0, 0}, nil
+       }
+
        buf := NewBuffer(nil)
        buf.WriteByte(byte(n.mask))

[henripqt]

@magiconair Alright I get it now.

Indeed it's not obvious at first glance! Should we provide builder functions to make it clear about requirements (especially for newcomers to the OPCUA world like myself :D ?

// NewBrowseRequest
func NewBrowseRequest(rh *RequestHeader, v *ViewDescription, n []*BrowseDescription, mx uint32) *BrowseRequest {
	return &BrowseRequest{
		RequestHeader:                 rh,
		View:                          v,
		NodesToBrowse:                 n,
		RequestedMaxReferencesPerNode: mx,
	}
}

As for the code comments, you're right all along. This is something that my golangci-lint usually catches for me and I've learned to rely a bit too much on it. This was some dumb code to facilitate reproducing the issue, nonetheless thank you for reminding me about it :)

[magiconair]

We have builders for this in the client. We might want to add a comment to the c.Browse function that you probably want to use the node.References method instead. I think the client.Browse could be a bit more robust in providing defaults.

[henripqt]

@magiconair Alright thanks. I'll try to explore the package more in-depth 👍

[magiconair]

I am closing this one since I've created a couple of PRs (#507, #508, and #509) as a result of this issue. None of them will fix it though. They will only make it harder for users to run into the same issue.