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.

Sign up for GitHub

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

Jump to bottom

Data Race in SecureChannel #184

Closed
ghost opened this issue May 7, 2019 · 7 comments · Fixed by #186
Closed

Data Race in SecureChannel #184

ghost opened this issue May 7, 2019 · 7 comments · Fixed by #186
Assignees
@magiconair
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented May 7, 2019

Doing subscribe, then publish, then BrowseRequest.

==================
WARNING: DATA RACE
Read at 0x00c000132568 by goroutine 62:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:236 +0x2b4
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Publish()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:451 +0x1ea

Previous write at 0x00c000132568 by goroutine 46:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:236 +0x2d4
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Browse()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:393 +0x83
  my-project/vendor/github.com/gopcua/opcua.(*Node).References()
      /go/src/my-project/vendor/github.com/gopcua/opcua/node.go:95 +0x3ef
(... project files)

==================
==================
WARNING: DATA RACE
Write at 0x00c0000d0aa0 by goroutine 62:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:237 +0x315
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Publish()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:451 +0x1ea

Previous write at 0x00c0000d0aa0 by goroutine 46:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:237 +0x315
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Browse()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:393 +0x83
  my-project/vendor/github.com/gopcua/opcua.(*Node).References()
      /go/src/my-project/vendor/github.com/gopcua/opcua/node.go:95 +0x3ef
(... project files)

==================
==================
WARNING: DATA RACE
Read at 0x00c0000d0ac0 by goroutine 62:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:238 +0x371
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Publish()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:451 +0x1ea

Previous write at 0x00c0000d0ac0 by goroutine 46:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:238 +0x391
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Browse()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:393 +0x83
  my-project/vendor/github.com/gopcua/opcua.(*Node).References()
      /go/src/my-project/vendor/github.com/gopcua/opcua/node.go:95 +0x3ef
(... project files)

==================
==================
WARNING: DATA RACE
Write at 0x00c0000d0aa8 by goroutine 62:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:239 +0x404
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Publish()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:451 +0x1ea

Previous write at 0x00c0000d0aa8 by goroutine 46:
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).SendAsync()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:239 +0x404
  my-project/vendor/github.com/gopcua/opcua/uasc.(*SecureChannel).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/uasc/secure_channel.go:204 +0x6c
  my-project/vendor/github.com/gopcua/opcua.(*Client).Send()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:315 +0xa5
  my-project/vendor/github.com/gopcua/opcua.(*Client).Browse()
      /go/src/my-project/vendor/github.com/gopcua/opcua/client.go:393 +0x83
  my-project/vendor/github.com/gopcua/opcua.(*Node).References()
      /go/src/my-project/vendor/github.com/gopcua/opcua/node.go:95 +0x3ef
(... project files)
==================

Debug logs

debug: Connect to opc.tcp://...
debug: conn 1: start HEL/ACK handshake
debug: conn 1: sent HELF with 61 bytes
debug: conn 1: recv ACKF with 28 bytes
debug: conn 1: recv &uacp.Acknowledge{Version:0x0, ReceiveBufSize:0xffff, SendBufSize:0xffff, MaxMessageSize:0x1000000, MaxChunkCount:0x101}
debug: conn 1/0: send *ua.OpenSecureChannelRequest with 132 bytes
debug: conn 1: recv OPNF with 136 bytes
debug: conn 1/0: set secure channel id to 779376701
debug: conn 1/0: recv OPNF with 136 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.OpenSecureChannelResponse
debug: conn 1/0: send *ua.CreateSessionRequest with 228 bytes
debug: conn 1: recv MSGF with 11328 bytes
debug: conn 1/0: recv MSGF with 11328 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.CreateSessionResponse
debug: conn 1/0: send *ua.ActivateSessionRequest with 117 bytes
debug: conn 1: recv MSGF with 96 bytes
debug: conn 1/0: recv MSGF with 96 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.ActivateSessionResponse
debug: conn 1/0: send *ua.CreateSubscriptionRequest with 84 bytes
debug: conn 1: recv MSGF with 72 bytes
debug: conn 1/0: recv MSGF with 72 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.CreateSubscriptionResponse
debug: conn 1/0: send *ua.BrowseRequest with 103 bytes
debug: conn 1/0: send *ua.PublishRequest with 66 bytes
debug: conn 1: recv MSGF with 206 bytes
debug: conn 1/0: recv MSGF with 206 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.BrowseResponse
debug: conn 1/0: send *ua.PublishRequest with 66 bytes
debug: conn 1: recv MSGF with 85 bytes
debug: conn 1/0: recv MSGF with 85 bytes
debug: conn 1/0: res: OK (0x0)
debug: conn 1/0: recv *ua.PublishResponse
debug: conn 1/0: send *ua.PublishRequest with 66 bytes
debug: conn 1: recv MSGF with 52 bytes
debug: conn 1/0: recv MSGF with 52 bytes
@ghost ghost mentioned this issue May 7, 2019
Closed
@ghost
Copy link
Author

ghost commented May 7, 2019

its this block

	s.cfg.SequenceNumber++
	s.reqhdr.AuthenticationToken = authToken
	s.reqhdr.RequestHandle++
	s.reqhdr.Timestamp = time.Now()

@magiconair
Copy link
Member

I'll have a look.

@magiconair magiconair self-assigned this May 7, 2019
@magiconair magiconair added the bug Something isn't working label May 7, 2019
@magiconair
Copy link
Member

@dwhutchison do you know whether it is crucial that the sequence number and request id don't have gaps or is it sufficient that they just increase?

magiconair added a commit that referenced this issue May 7, 2019
@magiconair
Issue #184: fix data race in SendAsync
38df7fa 
This is a first attempt to fix the data race in SendAsync.

Fixes #184
@magiconair magiconair mentioned this issue May 7, 2019
Merged
@magiconair
Copy link
Member

@wsnotify I've pushed a simple fix for this. Could you test whether this fixes the race? I need to think about this a bit more.

@dwhutchison
Copy link
Collaborator

Sequence number needs to increase by one on each request and reset to <1024 when it reaches the limit of a uint32-1024. It's a security check to make sure there's nobody masquerading as the real sender. The spec mentions having a reasonable allowance for gaps to account for dropped packets and temporary comms loss, but I would expect servers to disconnect clients if the series jumped more than 3-5 at once. If I can find a test case or other documentation to give a more firm number, I'll update this comment.

I don't believe request ID has the same restriction and simply needs to be unique for each request so the server can keep track of chunks.

@ghost
Copy link
Author

ghost commented May 8, 2019

@magiconair with this fix does not see races in my tests.

magiconair added a commit that referenced this issue May 8, 2019
@magiconair
Merge pull request #186 from gopcua/issue-184-send-async-race
b11b575 
Issue #184: fix data race in SendAsync
@magiconair magiconair mentioned this issue May 8, 2019
Closed
@magiconair
Copy link
Member

I've merged the fix so that your tests don't break and added #189 to follow-up on the sequence number description from @dwhutchison.

@alexbrdn alexbrdn mentioned this issue Jun 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@magiconair magiconair

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue #184: fix data race in SendAsync gopcua/opcua
2 participants
@magiconair @dwhutchison