Skip to content

Conversation

matt335672
Copy link
Member

This is the last of a couple of pull requests to address the memory issues found in #104 by @keithrob91.

As mentioned in that request, the clientCon is freed whenever rdpClientConSend() or rdpClientConRecv() detect a connection problem. Not all the code paths are expecting this, and as a result the clientCon is potentially accessed after being freed.

One approach would be to go through all the code paths which call rdpClientConSend() or rdpClientConRecv() and check they can handle the connection being deleted. I think this is likely to be a bit fiddly and quite brittle to maintain.

The approach I've taken is to remove most of the calls to rdpClientConDisconnect() and replace them with clearing a boolean flag connected . The actual call to rdpClientConDisconnect() now happens in the main select loop. This makes it a lot easier to check that the connection isn't accessed after being freed.

Here are some other notes:-

  • I noticed that struct _rdpClientCon contained a couple of boolean members that were effectively serving no purpose. These were connected and sckClosed. I've repurposed connected to act as the marker above, and I've removed sckClosed.
  • I've made the client connection list in the dev a double-linked list now rather than a single one. This simplifies the deletion of a connection from the list, as the list does not need to be scanned to find the connection.
  • connection addition and deletion logic is now in two routines rdpAddClientConToDev() and rdpRemoveClientConFromDev()
  • Around line 1173 I've replaced a while loop with a for loop over the list which simplifies the next element logic slightly. I couldn't find anything in the coding standards about these kind of for loops, but I know some project leads don't like them.

@keithrob91 - would you be happy to run your static analysis tool again? I don't have access to anything better than cppcheck here.

@metalefty
Copy link
Member

@jsorg71 @speidy LTGM, any thoughts?

@metalefty
Copy link
Member

Let's merge and test.

@metalefty metalefty merged commit c42b629 into neutrinolabs:devel Aug 2, 2018
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Aug 10, 2018
Backport these fixes for invalid memory access:
- neutrinolabs/xorgxrdp#124
- neutrinolabs/xorgxrdp#125

Approved by:	hrs (mentor)
Obtained from:	upstream
Relnotes:	https://github.com/neutrinolabs/xorgxrdp/releases/tag/v0.2.7
Sponsored by:	HAW International, Inc.
Differential Revision:	https://reviews.freebsd.org/D16601


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@476807 35697150-7ecd-e111-bb59-0022644237b5
uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Aug 10, 2018
Backport these fixes for invalid memory access:
- neutrinolabs/xorgxrdp#124
- neutrinolabs/xorgxrdp#125

Approved by:	hrs (mentor)
Obtained from:	upstream
Relnotes:	https://github.com/neutrinolabs/xorgxrdp/releases/tag/v0.2.7
Sponsored by:	HAW International, Inc.
Differential Revision:	https://reviews.freebsd.org/D16601
Jehops pushed a commit to Jehops/freebsd-ports-legacy that referenced this pull request Aug 10, 2018
Backport these fixes for invalid memory access:
- neutrinolabs/xorgxrdp#124
- neutrinolabs/xorgxrdp#125

Approved by:	hrs (mentor)
Obtained from:	upstream
Relnotes:	https://github.com/neutrinolabs/xorgxrdp/releases/tag/v0.2.7
Sponsored by:	HAW International, Inc.
Differential Revision:	https://reviews.freebsd.org/D16601


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@476807 35697150-7ecd-e111-bb59-0022644237b5
swills pushed a commit to swills/freebsd-ports that referenced this pull request Aug 12, 2018
Backport these fixes for invalid memory access:
- neutrinolabs/xorgxrdp#124
- neutrinolabs/xorgxrdp#125

Approved by:	hrs (mentor)
Obtained from:	upstream
Relnotes:	https://github.com/neutrinolabs/xorgxrdp/releases/tag/v0.2.7
Sponsored by:	HAW International, Inc.
Differential Revision:	https://reviews.freebsd.org/D16601


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@476807 35697150-7ecd-e111-bb59-0022644237b5
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 this pull request may close these issues.

2 participants