gbdev/rgbds
1
0
Fork 0
mirror of https://github.com/gbdev/rgbds.git synced 2025-11-20 10:12:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix critical oversight in lexer buffer refilling

Browse Source 
Since the lexer buffer wraps, the refilling gets handled in two steps:
First, iff the buffer would wrap, the buffer is refilled until its end.
Then, if more characters are requested, that amount is refilled too.

An important detail is that `read()` may not return as many characters as
requested; for this reason, the first step checks if its `read()` was
"full", and skips the second step otherwise.
This is also where a bug lied.

After a *lot* of trying, I eventually managed to reproduce the bug on an
OpenBSD VM, and after adding a couple of `assert`s in `peekInternal`, this
is what happened, starting at line 724:

0. `lexerState->nbChars` is 0, `lexerState->index` is 19;
1. We end up with `target` = 42, and `writeIndex` = 19;
2. 42 + 19 is greater than `LEXER_BUF_SIZE` (= 42), so the `if` is entered;
3. Within the first `readChars`, **`read` only returns 16 bytes**,
   advancing `writeIndex` to 35 and `target` to 26;
4. Within the second `readChars`, a `read(26)` is issued, overflowing the
   buffer.

The bug should be clear now: **the check at line 750 failed to work!** Why?
Because `readChars` modifies `writeIndex`.
The fix is simply to cache the number of characters expected, and use that.
This commit is contained in:
ISSOtm
2020-10-04 16:10:32 +02:00
parent c24694233f
commit 2eca43cd2d
1 changed files with 6 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/asm/lexer.c
View File

@@ -729,6 +729,8 @@ static int peekInternal(uint8_t distance)
ssize_t nbCharsRead = 0, totalCharsRead = 0; ssize_t nbCharsRead = 0, totalCharsRead = 0;
#define readChars(size) do { \ #define readChars(size) do { \
/* This buffer overflow made me lose WEEKS of my life. Never again. */ \
assert(writeIndex + (size) <= LEXER_BUF_SIZE); \
nbCharsRead = read(lexerState->fd, &lexerState->buf[writeIndex], (size)); \ nbCharsRead = read(lexerState->fd, &lexerState->buf[writeIndex], (size)); \
if (nbCharsRead == -1) \ if (nbCharsRead == -1) \
fatalerror("Error while reading \"%s\": %s\n", lexerState->path, errno); \ fatalerror("Error while reading \"%s\": %s\n", lexerState->path, errno); \
@@ -741,9 +743,11 @@ static int peekInternal(uint8_t distance)
/* If the range to fill passes over the buffer wrapping point, we need two reads */ /* If the range to fill passes over the buffer wrapping point, we need two reads */
if (writeIndex + target > LEXER_BUF_SIZE) { if (writeIndex + target > LEXER_BUF_SIZE) {
readChars(LEXER_BUF_SIZE - writeIndex); size_t nbExpectedChars = LEXER_BUF_SIZE - writeIndex;
readChars(nbExpectedChars);
/* If the read was incomplete, don't perform a second read */ /* If the read was incomplete, don't perform a second read */
if (nbCharsRead < LEXER_BUF_SIZE - writeIndex) if (nbCharsRead < nbExpectedChars)
target = 0; target = 0;
} }
if (target != 0) if (target != 0)