From 2eca43cd2da5a4be21cada359bd5e152f1896453 Mon Sep 17 00:00:00 2001 From: ISSOtm Date: Sun, 4 Oct 2020 16:10:32 +0200 Subject: [PATCH] Fix critical oversight in lexer buffer refilling 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. --- src/asm/lexer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/asm/lexer.c b/src/asm/lexer.c index 62904e29..146dd2c5 100644 --- a/src/asm/lexer.c +++ b/src/asm/lexer.c @@ -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); \ @@ -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)