[PATCH] Prevent DilloPlain::write() from reading past end of buffer
In src/plain.cc, \r\n sequences are handled with these lines: while ( i < MaxBytes ) { ... addLine(Start + i - len, len); if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; if (i < MaxBytes) ++i; ... So if a \r occurs, skip over the \n if present. But if i + 1 == MaxBytes, Start[i + 1] is one past the end of the buffer, which is an out-of-bounds read. This happens if a file ends in \r, or I suppose if a file gets broken up and parsed with multiple invocations of the function. I think this could theoretically add a redundant line break to the rendered page. My diff fixes this. I have tested it with \r, \r\n, and \n line endings. I haven't been able to test how it behaves with plaintext pages that are parsed in multiple invocations, though. (Also, and this is unrelated but it bothers me: prefsparser.cc has a comment saying the `symbols` array is sorted, but it isn't.) diff --git a/src/plain.cc b/src/plain.cc index 0f5d6a48..e085ab19 100644 --- a/src/plain.cc +++ b/src/plain.cc @@ -142,7 +142,10 @@ void DilloPlain::addLine(char *Buf, uint_t BufSize) { int len; char buf[129]; - char *end = Buf + BufSize; + char *end; + + if (BufSize > 0 && Buf[BufSize - 1] == '\r') --BufSize; + end = Buf + BufSize; currentLine++; /* Start at 1 */ sprintf(buf, "L%ld", currentLine); /* Always fits */ @@ -181,7 +184,8 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) while ( i < MaxBytes ) { switch ( state ) { case ST_SeekingEol: - if (Start[i] == '\n' || Start[i] == '\r') + if (Start[i] == '\n' || (Start[i] == '\r' && i + 1 < MaxBytes + && Start[i + 1] != '\n')) state = ST_Eol; else { ++i; ++len; @@ -189,8 +193,7 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) break; case ST_Eol: addLine(Start + i - len, len); - if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; - if (i < MaxBytes) ++i; + ++i; state = ST_SeekingEol; len = 0; break;
Hi, On Thu, Sep 10, 2026 at 02:21:53PM +0100, 256@256-32.com wrote:
In src/plain.cc, \r\n sequences are handled with these lines:
while ( i < MaxBytes ) { ... addLine(Start + i - len, len); if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; if (i < MaxBytes) ++i; ...
So if a \r occurs, skip over the \n if present. But if i + 1 == MaxBytes, Start[i + 1] is one past the end of the buffer, which is an out-of-bounds read. This happens if a file ends in \r, or I suppose if a file gets broken up and parsed with multiple invocations of the function. I think this could theoretically add a redundant line break to the rendered page.
My diff fixes this. I have tested it with \r, \r\n, and \n line endings. I haven't been able to test how it behaves with plaintext pages that are parsed in multiple invocations, though.
(Also, and this is unrelated but it bothers me: prefsparser.cc has a comment saying the `symbols` array is sorted, but it isn't.)
diff --git a/src/plain.cc b/src/plain.cc index 0f5d6a48..e085ab19 100644 --- a/src/plain.cc +++ b/src/plain.cc @@ -142,7 +142,10 @@ void DilloPlain::addLine(char *Buf, uint_t BufSize) { int len; char buf[129]; - char *end = Buf + BufSize; + char *end; + + if (BufSize > 0 && Buf[BufSize - 1] == '\r') --BufSize; + end = Buf + BufSize;
currentLine++; /* Start at 1 */ sprintf(buf, "L%ld", currentLine); /* Always fits */ @@ -181,7 +184,8 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) while ( i < MaxBytes ) { switch ( state ) { case ST_SeekingEol: - if (Start[i] == '\n' || Start[i] == '\r') + if (Start[i] == '\n' || (Start[i] == '\r' && i + 1 < MaxBytes + && Start[i + 1] != '\n')) state = ST_Eol; else { ++i; ++len; @@ -189,8 +193,7 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) break; case ST_Eol: addLine(Start + i - len, len); - if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; - if (i < MaxBytes) ++i; + ++i;
Why not just this? diff --git a/src/plain.cc b/src/plain.cc index 0f5d6a48..343cc05c 100644 --- a/src/plain.cc +++ b/src/plain.cc @@ -189,7 +189,8 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) break; case ST_Eol: addLine(Start + i - len, len); - if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; + if (Start[i] == '\r' && i + 1 < MaxBytes && Start[i + 1] == '\n') + ++i; if (i < MaxBytes) ++i; state = ST_SeekingEol; len = 0; Best, Rodrigo.
DilloPlain:write() is written in such a way that it can receive a file one part at a time, rather than all at once. As such, it might receive "line 1\r" and then "\nline 2". I'm not sure if that actually happens in practise, and I haven't been able to provoke this behaviour to test it (piping bytes into netcat with a sleep delay didn't do it), but I think the old code would turn that \r\n into 2 line breaks instead of 1, because the function is called separately for both parts. I hope that makes sense. Most of the complexity of my patch consists of dealing with that. Maybe there's still a simpler way to deal with it, though.
Hi, On Mon, Oct 12, 2026 at 05:55:08PM +0100, 256@256-32.com wrote:
DilloPlain:write() is written in such a way that it can receive a file one part at a time, rather than all at once. As such, it might receive "line 1\r" and then "\nline 2". I'm not sure if that actually happens in practise, and I haven't been able to provoke this behaviour to test it (piping bytes into netcat with a sleep delay didn't do it), but I think the old code would turn that \r\n into 2 line breaks instead of 1, because the function is called separately for both parts.
I hope that makes sense.
Oh, I see. Thanks for the clarification!
Most of the complexity of my patch consists of dealing with that. Maybe there's still a simpler way to deal with it, though.
Yeah, I don't like that much how this function is designed, is trying to do too many things at once. Do you have something in mind to try to make it simpler? I opened https://bug.dillo-browser.org/536/ to track it. Thanks!, Rodrigo.
I don't have a whole redesign of the function in mind, but I thought about it some more and now I have a more elegant patch that I think fixes the problem. I made the second `++i` unconditional because I think `i` is now guaranteed to be less than MaxBytes at that point: diff --git a/src/plain.cc b/src/plain.cc index 0f5d6a48..0770af92 100644 --- a/src/plain.cc +++ b/src/plain.cc @@ -176,6 +176,14 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) _MSG("DilloPlain::write Eof=%d\n", Eof); Start = (char*)Buf + Start_Ofs; + + if (Start_Ofs > 0 && Start_Ofs < BufSize && + Start[-1] == '\r' && Start[0] == '\n'){ + // Prevent \r\n from resulting in 2 line breaks. + ++Start_Ofs; + ++Start; + } + MaxBytes = BufSize - Start_Ofs; i = len = 0; while ( i < MaxBytes ) { @@ -189,8 +197,8 @@ void DilloPlain::write(void *Buf, uint_t BufSize, int Eof) break; case ST_Eol: addLine(Start + i - len, len); - if (Start[i] == '\r' && Start[i + 1] == '\n') ++i; - if (i < MaxBytes) ++i; + if (Start[i] == '\r' && i + 1 < MaxBytes && Start[i + 1] == '\n') ++i; + ++i; state = ST_SeekingEol; len = 0; break;
participants (2)
-
256＠256-32.com -
Rodrigo Arias