You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by aiden0z <gi...@git.apache.org> on 2018/08/23 07:24:36 UTC
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
GitHub user aiden0z opened a pull request:
https://github.com/apache/guacamole-server/pull/181
GUACAMOLE-614: fix infinite loop in guac_iconv
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
```console
[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
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aiden0z/guacamole-server master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-server/pull/181.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #181
----
commit 32214ccc8e8c1b897b782795514d08b373f2876b
Author: Aiden Luo <ai...@...>
Date: 2018-08-23T07:20:19Z
GUACAMOLE-614: fix infinite loop in guac_iconv
----
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212224559
--- Diff: src/common/iconv.c ---
@@ -73,11 +73,19 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
+ /* Stop if not enough bytes to read */
+ if (*input - read_start == 0) {
+ return 1;
+ }
in_remaining -= *input - read_start;
/* Write character */
write_start = *output;
writer(output, out_remaining, value);
+ /* Stop if not enough room */
+ if (*output - write_start == 0 ) {
--- End diff --
Same thing as above - when you only have one line of code executing inside the `if` statement, no need for the braces.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212509893
--- Diff: src/common/iconv.c ---
@@ -67,18 +67,27 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
while (in_remaining > 0 && out_remaining > 0) {
int value;
+ int count;
const char* read_start;
char* write_start;
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
- in_remaining -= *input - read_start;
+ count = *input - read_start;
+ /* Stop if not enough bytes to read */
+ if (count == 0)
+ return 1;
--- End diff --
The return value for `guac_iconv()` is documented as:
> Non-zero if the NULL terminator of the input string was read and copied into the destination string, zero otherwise.
If the conversion operation is being aborted prior to converting the entire input string, zero should be returned. A non-zero value should be used only when conversion has succeeded.
You could achieve the same here just with `break`.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by aiden0z <gi...@git.apache.org>.
Github user aiden0z commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r213258980
--- Diff: src/common/iconv.c ---
@@ -67,18 +67,31 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
while (in_remaining > 0 && out_remaining > 0) {
int value;
+ int count;
const char* read_start;
char* write_start;
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
- in_remaining -= *input - read_start;
+ count = *input - read_start;
+
+ /* Stop if read zero byte */
+ if (count == 0)
+ break;
+
+ in_remaining -= count;
/* Write character */
write_start = *output;
writer(output, out_remaining, value);
- out_remaining -= *output - write_start;
+ count = *output - write_start;
+
+ /* Stop if write zero byte */
--- End diff --
I think the root cause is not the zero byte but the random value of the uninitialized int variable in `GUAC_READ_UTF8 `.
```c
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.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212509092
--- Diff: src/common/iconv.c ---
@@ -67,18 +67,27 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
while (in_remaining > 0 && out_remaining > 0) {
int value;
+ int count;
const char* read_start;
char* write_start;
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
- in_remaining -= *input - read_start;
+ count = *input - read_start;
+ /* Stop if not enough bytes to read */
+ if (count == 0)
+ return 1;
+ in_remaining -= count;
/* Write character */
write_start = *output;
writer(output, out_remaining, value);
- out_remaining -= *output - write_start;
+ count = *output - write_start;
+ /* Stop if not enough room */
--- End diff --
Please include a blank line before comments. The comment (and subsequent code block) becomes difficult to distinguish from the rest of the code otherwise.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212501403
--- Diff: src/common/iconv.c ---
@@ -67,18 +67,27 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
while (in_remaining > 0 && out_remaining > 0) {
int value;
+ int count;
const char* read_start;
char* write_start;
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
- in_remaining -= *input - read_start;
+ count = *input - read_start;
+ /* Stop if not enough bytes to read */
--- End diff --
This test is checking whether the reader successfully read a character, not whether enough bytes were read.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212780321
--- Diff: src/common/iconv.c ---
@@ -67,18 +67,31 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
while (in_remaining > 0 && out_remaining > 0) {
int value;
+ int count;
const char* read_start;
char* write_start;
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
- in_remaining -= *input - read_start;
+ count = *input - read_start;
+
+ /* Stop if read zero byte */
+ if (count == 0)
+ break;
+
+ in_remaining -= count;
/* Write character */
write_start = *output;
writer(output, out_remaining, value);
- out_remaining -= *output - write_start;
+ count = *output - write_start;
+
+ /* Stop if write zero byte */
--- End diff --
Before moving forward with this, can you clarify why writing zero bytes should universally be considered a failure condition?
It definitely makes sense to bail out if no bytes are read, as that will result in an infinite parse loop, but it's not necessarily illegal for a writer to choose not to write any output for a given input. The only case where that would *definitely* be considered a failure would be if `value` is 0, as `guac_iconv()` should always fail if it cannot write a null terminator.
---
[GitHub] guacamole-server pull request #181: GUACAMOLE-614: fix infinite loop in guac...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-server/pull/181#discussion_r212224304
--- Diff: src/common/iconv.c ---
@@ -73,11 +73,19 @@ int guac_iconv(guac_iconv_read* reader, const char** input, int in_remaining,
/* Read character */
read_start = *input;
value = reader(input, in_remaining);
+ /* Stop if not enough bytes to read */
+ if (*input - read_start == 0) {
--- End diff --
To keep style consistent across the code, and since this is a single line of code in the `if` statement, there's no reason for the braces.
---