New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

readUntilSmall may read over separator [critical] #1741

Closed
jpf91 opened this Issue Apr 13, 2017 · 0 comments

Comments

Projects
None yet
1 participant
@jpf91
Contributor

jpf91 commented Apr 13, 2017

Under certain conditions readUntilSmall might read over an separator.
Here's a debug log, matching vibe.D 0.7.31.

Pasting the relevant code:

while (true) {
    auto max_peek = max(max_bytes, max_bytes+nmarker); // account for integer overflow
    auto pm = stream.peek()[0 .. min($, max_bytes)];
    if (!pm.length) { // no peek support - inefficient route
        ubyte[2] buf = void;
        auto l = nmarker - nmatched;
        stream.read(buf[0 .. l]);
        foreach (i; 0 .. l) {
            if (buf[i] == end_marker[nmatched]) {
                nmatched++;
            } else if (buf[i] == end_marker[0]) {
                foreach (j; 0 .. nmatched) dst.put(end_marker[j]);
                nmatched = 1;
            } else {
                foreach (j; 0 .. nmatched) dst.put(end_marker[j]);
                nmatched = 0;
                dst.put(buf[i]);
            }
            if (nmatched == nmarker) return;
        }
    } else {
        auto idx = pm.countUntil(end_marker[0]);
        if (idx < 0) {
            dst.put(pm);
            max_bytes -= pm.length;
            stream.skip(pm.length);
        } else {
            dst.put(pm[0 .. idx]);
            stream.skip(idx+1);
            if (nmarker == 2) {
                ubyte[1] next;
                stream.read(next);
                if (next[0] == end_marker[1])
                    return;
                dst.put(end_marker[0]);
                dst.put(next[0]);
            } else return;
        }
    }
}

Considering this input string in the stream: 0\r\n, separator \r\n.

In the first iteration there's never been a call to read or leastSize (i.e. the blocking functions) so the internal buffer is empty, peek returns []. Because of that the slow path is taken and calls read. read returns two bytes but internally fills the buffer with more data. Dst is set to "0" and nmatched is 1 as the \r has been found.

The loop continues in the next iteration. This time peek returns a cached buffer and the fast path is taken. The fast block now searches for end_marker[0], even though nmatched == 1 and it should search for end_marker[nmatched].

I reproduced this with a custom stream implementation, but I think this could happen for any stream.

I've switched to one-byte \n separators as a workaround, but this bug could be critical for other applications.

s-ludwig added a commit that referenced this issue May 14, 2017

s-ludwig added a commit that referenced this issue May 14, 2017

Fix interleaving fast and slow paths in readUntilSmall. Fixes #1741.
Also fixes handling of the max_bytes parameter in the fast path.

s-ludwig added a commit that referenced this issue May 14, 2017

Fix interleaving fast and slow paths in readUntilSmall. Fixes #1741.
Also fixes handling of the max_bytes parameter in the fast path.

s-ludwig added a commit that referenced this issue May 14, 2017

s-ludwig added a commit that referenced this issue May 14, 2017

Fix interleaving fast and slow paths in readUntilSmall. Fixes #1741.
Also fixes handling of the max_bytes parameter in the fast path.

s-ludwig added a commit that referenced this issue May 14, 2017

s-ludwig added a commit that referenced this issue May 14, 2017

Fix interleaving fast and slow paths in readUntilSmall. Fixes #1741.
Also fixes handling of the max_bytes parameter in the fast path.

@s-ludwig s-ludwig closed this in #1761 May 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment