Generalise development build options, and add --enable-devel-streamcheck

[matt335672]

This [draft] PR which adds a new development-only option --enable-streamcheck

Essentially it adds a check after every stream operation in parse.h that all is working as expected. If not a dump is taken. So it will impact performance, but should let us catch and find stream violations relatively easily during development. I'm not intending for this to be used at runtime!

It seemed to me it would be useful when working on #1885.

I've found a few gremlins already which I'm working through so this isn't yet ready. However, at this stage I'd appreciate any thoughts people have on this.

[aquesnel]

Hi @matt335672

This looks good. I'd also suggest adding the sanitize compile flags to catch more memory errors. Here's my attempt at adding the sanitize flags: aquesnel@27a0eaf

[aquesnel]

The parse.c file is missing from this commit.

[matt335672]

Oops. Should be there now!

[Nexarian]

Might I suggest taking a page from FreeRDP's book: https://github.com/FreeRDP/FreeRDP/blob/master/cmake/ConfigOptions.cmake#L100

Essentially: Options that are for debugging are explicitly labeled so, and you also have the ability to toggle all debug options.

[matt335672]

Thanks for that.

I like the idea a lot, but we already have the --enable-xrdpdebug option which does the following (AFAICT):-

• Adds extra checking and logging to the embedded pixman code.
• Defines the LOG_DEVEL() etc macros to do something interesting
• Replaces the compiler flag -O2 with -g -O0.

So my question is; how do we get from where we are to where we want to be?

One idea is just to use a single --enable-xrdpdebug flag for everything. I'm not keen on this, as the checking above, and also @aquesnel's sanitise suggestion cause program exits when they find problems. This might be OK when everything is bedded in, but it will take a while to get there I think. Or am I being too critical?

Here's an other idea, which can be improved on I'm sure.

• Rename --enable-xrdp-debug as --enable-debug-logging (I'm not so bothered about renaming a developer-only option)
• Rename the option above to --enable-debug-stream
• Add a --enable-debug-all to enable all debug options. Or re-use the --enable-xrdpdebug flag for this.
• If any debug option is enabled, that sets compiler debug flags.

This is more disruptive but gives an extensible system it should be easy to plug other things into (e.g. the sanitize options).

Thoughts, anyone?

[aquesnel]

I agree with your suggestion to split up the debug config flags.

[metalefty]

I'm not so bothered about renaming a developer-only option

I agree.

[matt335672]

Thanks to all for comments. This is now ready for review. I've split it into 5 commits:-

1. Split the existing --enable-xrdpdebug into --enable-devel-all, --enable-devel-debug, --enable-devel-logging.
2. Add the additional --enable-devel-streamcheck option.
3. Fix some unterminated buffer problems in the xrdp file I/O code found by --enable-devel-streamcheck. There's a one-liner to sesman in here too which dosn't warrant a separate commit.
4. Fix a couple of minor problems in the RDP layer in librdp. One was a too-small size, and the other was an unitialised sec_hdr pointer which caused an out-of-bounds (but unused) pointer when the stream was compressed.
5. Add some stream terminations to chansrv

Configure options are now as follows:-

Option	Meaning
--enable-devel-debug	Ensures executables are optimised for a debugger
--enable-devel-logging	Mostly enables the LOG_DEVEL() etc macros
--enable-devel-streamcheck	Forces range-checking on all stream operations
--enable-devel-all	Enables all development options

The way it's set up means that combinations like ./configure --enable-devel-all --disable-devel-streamcheck work if you just want to disable a single feature which is giving you trouble.

I've moved the necessary pre-processor options into config_ac.h, which means I've removed a lot of these kind of stanzas from Makefile.am files:-

if XRDP_DEBUG
AM_CPPFLAGS += -DXRDP_DEBUG
endif

I haven't added @aquesnel's suggested sanitiser calls yet, but it should be trivial to do so. Maybe a separate commit?

[aquesnel]

I'd say that we can add the sanitize flags later in a separate PR.
What are the next steps to get this PR merged?

[matt335672]

I could really do with this for testing the stuff I've got in #1900. Since it's developer-only I'll merge it now.