Skip to content

Commit

Permalink
Fix critical oversight in lexer buffer refilling
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ISSOtm committed Oct 4, 2020
1 parent c246942 commit 2eca43c
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/asm/lexer.c
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,8 @@ static int peekInternal(uint8_t distance)
ssize_t nbCharsRead = 0, totalCharsRead = 0;

#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)); \
if (nbCharsRead == -1) \
fatalerror("Error while reading \"%s\": %s\n", lexerState->path, errno); \
Expand All @@ -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 (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 (nbCharsRead < LEXER_BUF_SIZE - writeIndex)
if (nbCharsRead < nbExpectedChars)
target = 0;
}
if (target != 0)
Expand Down

0 comments on commit 2eca43c

Please sign in to comment.