On Sun, Jan 10, 2021 at 7:57 AM Jason Keltz <[email protected]> wrote: > On 1/10/2021 12:26 AM, Mike Jumper wrote: > > On Sat, Jan 9, 2021 at 8:43 PM Mike Jumper <[email protected]> wrote: > >> On Sat, Jan 9, 2021 at 4:50 PM Mike Jumper <[email protected]> wrote: >> >>> On Sat, Jan 9, 2021 at 6:38 AM Jason Keltz <[email protected]> wrote: >>> >>>> On 1/9/2021 4:40 AM, Mike Jumper wrote: >>>> >>>> ... >>>> >>>> Hi Mike, >>>> >>>> Thanks. My system is running CentOS 7.8 so it wasn't affected by the >>>> move to FreeRDP 2.1.1 (which happened in 7.9 I believe). In addition, when >>>> I went back to 1.2.0, since I had overwritten guacd with my >>>> guacamole-server-1.3.0 install, I had to recompile guacamole-server-1.2.0, >>>> and it worked just as before. The problem has to be something else. >>>> >>> >>> The changes to the RDP support which detect whether FreeRDP will >>> automatically free the relevant memory were introduced in 1.3.0. Reverting >>> to 1.2.0 would not include the changes that would result in a double-free >>> if the build detects one type of behavior but the library installed >>> actually does another. In the event that the library installed does not >>> automatically free memory (ie: if 2.0.0-rc4 is installed and then things >>> are upgraded to 2.1.1), this would mean that 1.2.0 would leak. >>> >>> See: >>> >>> https://issues.apache.org/jira/browse/GUACAMOLE-1181 >>> https://issues.apache.org/jira/browse/GUACAMOLE-1241 >>> >> >> Welp, testing myself on a fresh CentOS 7 machine and manually installing >> the older 2.0.0-rc4 packages, I see the same double-free behavior. I'm not >> sure what's happening here, as both frees appear to be happening within >> FreeRDP: >> >> guacd[14986]: INFO: Guacamole proxy daemon (guacd) version 1.3.0 started >> guacd[14986]: INFO: Listening on host 127.0.0.1, port 4822 >> guacd[14986]: INFO: Creating new client for protocol "rdp" >> guacd[14986]: INFO: Connection ID is >> "$825f8c0f-2bff-4a88-87ad-a013711a7f9f" >> guacd[14988]: INFO: Security mode: TLS >> guacd[14988]: INFO: Resize method: display-update >> guacd[14988]: INFO: Loading keymap "base" >> guacd[14988]: INFO: Loading keymap "en-us-qwerty" >> guacd[14988]: INFO: User "@fc3f00fc-aa20-4ffb-a9dc-b75f3e4e6cbf" joined >> connection "$825f8c0f-2bff-4a88-87ad-a013711a7f9f" (1 users now present) >> guacd[14988]: INFO: Connected to RDPDR 1.13 as client 0x0005 >> ==14988== Thread 4: >> ==14988== Invalid read of size 4 >> ==14988== at 0xCB5B4CD: Stream_Free (stream.c:130) >> ==14988== by 0xC5E911C: channel_queue_message_free.part.0 >> (client.c:94) <-- Free #2 >> ==14988== by 0xC5E9711: freerdp_channels_process_sync.isra.2 >> (client.c:503) >> ==14988== by 0xC5EA1B4: freerdp_channels_check_fds (client.c:573) >> ==14988== by 0xC5E7D9E: freerdp_check_event_handles (freerdp.c:388) >> ==14988== by 0xC34548F: guac_rdp_handle_connection (rdp.c:500) >> ==14988== by 0xC34548F: guac_rdp_client_thread (rdp.c:747) >> ==14988== by 0x5A18EA4: start_thread (in /usr/lib64/libpthread-2.17.so >> ) >> ==14988== by 0x660C96C: clone (in /usr/lib64/libc-2.17.so) >> ==14988== Address 0x1615a8f4 is 52 bytes inside a block of size 56 free'd >> ==14988== at 0x4C2B06D: free (vg_replace_malloc.c:540) >> ==14988== by 0xC5E974A: freerdp_channels_process_sync.isra.2 >> (client.c:497) <-- Free #1 >> ==14988== by 0xC5EA1B4: freerdp_channels_check_fds (client.c:573) >> ==14988== by 0xC5E7D9E: freerdp_check_event_handles (freerdp.c:388) >> ==14988== by 0xC34548F: guac_rdp_handle_connection (rdp.c:500) >> ==14988== by 0xC34548F: guac_rdp_client_thread (rdp.c:747) >> ==14988== by 0x5A18EA4: start_thread (in /usr/lib64/libpthread-2.17.so >> ) >> ==14988== by 0x660C96C: clone (in /usr/lib64/libc-2.17.so) >> >> >> The build *does* correctly detect the free behavior of 2.0.0-rc4: >> >> >> checking whether pVirtualChannelWriteEx() frees the wStream upon >> completion... yes >> >> >> While the valgrind results above suggest a problem with FreeRDP, the fact >> that things worked fine with 1.2.0 suggest otherwise ... yet the detected >> free behavior should result in the new guac-side free code being omitted. >> It should be identical to the behavior of 1.2.0. >> >> I'll retest with 1.2.0 and, assuming it does work, I'll do some bisecting >> and see whether something jumps out. This looks like a regression. >> > > Ugh ... OK. I found the problem. Everything is correctly detected, the > code that would cause this is correctly conditional ... all that is > absolutely fine. We're just not including config.h in the relevant file, so > the macro has no effect and the free always happens. > > I've opened: > > https://issues.apache.org/jira/browse/GUACAMOLE-1259 > > and: > > https://github.com/apache/guacamole-server/pull/325 > > If encountering this issue, the best option is to use a newer version of > FreeRDP. If this is not possible, the next best option is to just add the > missing include (see above PR). Alternatively, you can build with the > relevant macro manually defined by running: > > ./configure CFLAGS="-DFREERDP_SVC_CORE_FREES_WSTREAM" > > > when you rebuild. > > > Hi Mike, > > Thanks so much for checking into that for me so quickly! Very appreciated! > > I did try to compile with the CFLAGS options because it was the easiest > for a quick compile, but I was puzzled because it would not include the > missing "config.h". As I suspected would happen, I got various errors > about FREERDP_SVC_CORE_FREES_WSTREAM being redefined. It's possible I > wasn't supposed to do that. >
Right - never mind on that. Adding CFLAGS="-DFREERDP_SVC_CORE_FREES_WSTREAM" will not be useful in this case. As it's correctly present in config.h, specifying CFLAGS="-DFREERDP_SVC_CORE_FREES_WSTREAM" will result in redefinition errors when compiling any file that *does* include config.h. Manually specifying that macro would only be useful if (1) you're forced to use a dev version of FreeRDP that the Guacamole build cannot know the behavior of and (2) you happen to know that the particular version of FreeRDP automatically frees the wStream of sent SVC PDUs. It was just as easy to add the: #include "config.h" to > src/protocols/rdp/plugins/guac-common-svc/guac-common-svc.c as per your > patch, and this did indeed work. I managed to get the new 1.3.0 in version > before the beginning of the term tomorrow. > > So when I upgrade the guac server to CentOS 7.9, and have the latest > FreeRDP, I should only need to recompile guacamole-server and I shouldn't > get any errors - is that correct? > Yep. If you don't recompile, the RDP support will still be expecting FreeRDP to free the memory in question, resulting in a memory leak. Michael Jumper CEO, Lead Developer Glyptodon Inc <https://enterprise.glyptodon.com/>.
