GUACAMOLE-614: fix infinite loop in guac_iconv

[aiden0z]

The encoding reader/writer may return zero, but the caller did not check zero value. That may cause the infinite loop when handle incorrect bytes.

See the line starts with /*** .

int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
guac_iconv_write* writer, char** output, int out_remaining) {

while (in_remaining > 0 && out_remaining > 0) {

int value;
const char* read_start;
char* write_start;

/* Read character */
read_start = *input;


/*** reader may return value and not change input ***/
value = reader(input, in_remaining);


in_remaining -= *input - read_start;

/* Write character */
write_start = *output;

/*** writer may return value and not change output ***/
writer(output, out_remaining, value);


out_remaining -= *output - write_start;

/* Stop if null terminator reached */
if (value == 0)
return 1;

}

/* Null terminator not reached */
return 0;
}

Analysis procedure

1. Find problem thread

[root@jettydocker2 /]# ps -ef
UID PID PPID C STIME TTY TIME CMD
root 1 0 0 01:35 ? 00:00:40 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 464 1 0 02:51 ? 00:00:17 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 615 1 96 03:12 ? 01:23:39 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 703 1 0 03:38 ? 00:00:00 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 712 1 0 03:45 ? 00:00:15 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 728 1 0 04:01 ? 00:00:02 /usr/local/sbin/guacd -b 0.0.0.0 -f
root 735 0 0 04:39 ? 00:00:00 bash
root 747 735 0 04:39 ? 00:00:00 ps -ef

[root@jettydocker2 /]# top -b -H -p 615
top - 04:40:08 up 48 days, 2:56, 0 users, load average: 1.01, 1.11, 1.19
Threads: 3 total, 1 running, 2 sleeping, 0 stopped, 0 zombie
%Cpu(s): 25.4 us, 0.0 sy, 0.0 ni, 74.6 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st
KiB Mem : 8175596 total, 2269700 free, 1883596 used, 4022300 buff/cache
KiB Swap: 0 total, 0 free, 0 used. 5914664 avail Mem

PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
619 root 20 0 1861008 23848 3444 R 99.9 0.3 83:56.87 guacd
615 root 20 0 1861008 23848 3444 S 0.0 0.3 0:00.00 guacd
621 root 20 0 1861008 23848 3444 S 0.0 0.3 0:00.02 guacd

2. Identify problem code

[root@jettydocker2 /]# gdb /usr/local/sbin/guacd 615

GNU gdb (GDB) Red Hat Enterprise Linux 7.6.1-110.el7
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show
....

(gdb) info threads
Id Target Id Frame
3 Thread 0x7efffeffd700 (LWP 619) "guacd" guac_iconv (reader=0x7f00683e22fa <GUAC_READ_UTF8>, input=0x7efffefbc638, in_remaining=2,
writer=0x7f00683e240f <GUAC_WRITE_UTF8>, output=0x7efffefbc630, out_remaining=1) at iconv.c:74
2 Thread 0x7f0059ffb700 (LWP 621) "guacd" 0x00007f00721f9f3d in nanosleep () from /lib64/libpthread.so.0
* 1 Thread 0x7f0043fff700 (LWP 615) "guacd" 0x00007f00721f3f97 in pthread_join () from /lib64/libpthread.so.0
(gdb) t 3
[Switching to thread 3 (Thread 0x7efffeffd700 (LWP 619))]
#0 guac_iconv (reader=0x7f00683e22fa <GUAC_READ_UTF8>, input=0x7efffefbc638, in_remaining=2, writer=0x7f00683e240f <GUAC_WRITE_UTF8>,
output=0x7efffefbc630, out_remaining=1) at iconv.c:74
74 read_start = *input;

(gdb) bt
#0 guac_iconv (reader=0x7f00683e22fa <GUAC_READ_UTF8>, input=0x7efffefbc638, in_remaining=2, writer=0x7f00683e240f <GUAC_WRITE_UTF8>,
output=0x7efffefbc630, out_remaining=1) at iconv.c:74
#1 0x00007f00683df11d in guac_vnc_cut_text (client=0x7f007302e010, text=0x7f00600019d0 " #", <incomplete sequence \345\255>, textlen=5) at clipboard.c:135
#2 0x00007f00681c97c7 in HandleRFBServerMessage () from /lib64/libvncclient.so.0
#3 0x00007f00683e0c97 in guac_vnc_client_thread (data=0x7f005c00aee0) at vnc.c:350
#4 0x00007f00721f2e25 in start_thread () from /lib64/libpthread.so.0
#5 0x00007f0070aeabad in clone () from /lib64/libc.so.6

(gdb) f 0
#0 guac_iconv (reader=0x7f00683e22fa <GUAC_READ_UTF8>, input=0x7efffefbc638, in_remaining=2, writer=0x7f00683e240f <GUAC_WRITE_UTF8>,
output=0x7efffefbc630, out_remaining=1) at iconv.c:79
79 write_start = *output;
(gdb) n [18/1979]
80 writer(output, out_remaining, value);
(gdb) n
81 out_remaining -= *output - write_start;
(gdb) n
84 if (value == 0)
(gdb) n
67 while (in_remaining > 0 && out_remaining > 0) {
(gdb) n
74 read_start = *input;
(gdb) n
75 value = reader(input, in_remaining);
(gdb) step
GUAC_READ_UTF8 (input=0x7efffefbc638, remaining=2) at iconv.c:98
98 *input += guac_utf8_read(*input, remaining, &value);
(gdb) step
guac_utf8_read (utf8=0x7f00600019d3 <incomplete sequence \345\255>, length=2, codepoint=0x7efffefbc58c) at unicode.c:133
133 if (length <= 0)
(gdb) n
137 initial = (unsigned char) *(utf8++);
(gdb) n
140 if ((initial | 0x7F) == 0x7F) {
(gdb) n
146 else if ((initial | 0x1F) == 0xDF) {
(gdb) n
152 else if ((initial | 0x0F) == 0xEF) {
(gdb) n
153 result = initial & 0x0F;
(gdb) n
154 bytes = 3;
(gdb) n
170 if (bytes > length)
(gdb) n
171 return 0;
(gdb) n
182 }
GUAC_READ_UTF8 (input=0x7efffefbc638, remaining=2) at iconv.c:99
99 return value;
(gdb) n
101 }
(gdb)
guac_iconv (reader=0x7f00683e22fa <GUAC_READ_UTF8>, input=0x7efffefbc638, in_remaining=2, writer=0x7f00683e240f <GUAC_WRITE_UTF8>, output=0x7efffefbc630,
out_remaining=1) at iconv.c:76
76 in_remaining -= *input - read_start;
(gdb) p *input - read_start
$15 = 0

[necouchman]

@aiden0z Thank you for the very detailed problem report and the code contribution. In the future, the JIRA issue is the right place for all of the analysis, etc. - no need to duplicate that information here in the pull request :smile:.

[aiden0z]

Updated the code style.

[aiden0z]

Update comment and code style.

[aiden0z]

I think the root cause is not the zero byte but the random value of the uninitialized int variable in GUAC_READ_UTF8 .

int GUAC_READ_UTF8(const char** input, int remaining) {

    int value;

    *input += guac_utf8_read(*input, remaining, &value);
    return value;

}

guac_utf8_read will not update value (codepoint) when return zero. That may cause GUAC_READ_UTF8 always return a int value great than zero to guac_iconv because of the uninitialized local variable.

I will update the commits.

[mike-jumper]

The potential for GUAC_READ_UTF8 to return garbage is a good catch, as that could result in the converted string receiving garbage if invalid UTF-8 is given.

While that's worth fixing, isn't the original issue still present? Won't guac_iconv() still potentially get stuck in a busy loop if a reader like GUAC_READ_UTF8 does not update the pointer (if there is insufficient space to read the rest of the character)?