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. - Mike
