Invalid memory reads and writes

[keithrob91]

Hi,

I have run a static analysis tool on xorgxrdp which has shown up a few issues that I hope you don’t mind me reporting. Most of them relate to multiple invalid memory reads and writes, with most of them related to the same code pattern.

They relate to the use of rdpClientConPreCheck() and the fact that calling this function can free ‘clientCon’ and ‘clientCon->out_s’, and then these are used after the call to rdpClientConPreCheck().

For example:

Within rdpClientConSetOpcode() (rdpClientCon.c) the following occurs:

rdpClientConPreCheck ( dev, clientCon, 6 ); <--- clientCon->out_s and clientCon can both be free’d
out_uint16_le ( clientCon->out_s, 14 ); <--- clientCon and clientCon->out_s both accessed

It looks like most, if not all, of these occur under an error condition which causes rdpClientConDisconnect() to be called. This is the function that actually free’s the memory, and all subsequent accesses to this memory are flagged by the tool.

An example of an invalid memory write is within rdpClientConPreCheck() itself after the error condition has occurred and the following line is executed:

clientCon->count = 0;

Additionally, the tool is also reporting an array overrun in rdpmouseControl() when the function InitPointerDeviceStruct() is called. It is complaining that passing 9 for the numButtons argument with a map array of size 9 overruns the array. I suspect this is because InitButtonClassDeviceStruct(), which is called by InitPointerDeviceStruct(), performs a loop from 1 to numButtons and dereferences the array at map[9].

[metalefty]

Thanks for reporting! I'll investigate it when I have time. If you can provide a patch, I'll look at it.

[matt335672]

I've had a look at the excellent concerns @keithrob91 has raised with the static analysis tool.

The basic problem seems that whenever rdpClientConSend() or rdpClientConRecv() detect a connection problem they immediately call rdpClientConDisconnect() which frees the clientCon. it isn't immediately obvious when they are being called. As a result, clientCon could be used after being freed in several locations.

I've got a patch put together which I need to unit test before I submit it. What's the best way to do submit a patch for an issue? Just submit a pull request as usual?

[moobyfr]

yes, pull request is the easier method

[matt335672]

This was fixed a long time ago - closing