Skip to content
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

readbytes! on HTTP stream and IOBuffer should return the number of read bytes #915

Open
rfourquet opened this issue Aug 26, 2022 · 1 comment

Comments

@rfourquet
Copy link

This is what readbytes! docstring from Base says, but readbytes! on a HTTP streams returns the total number of bytes.
I realize that this method is probably internal as it takes an IOBuffer, so feel free to close this issue, but it wouldn't be a difficult change to update it, it could be:

function Base.readbytes!(http::Stream, buf::IOBuffer, n=bytesavailable(http))
    Base.ensureroom(buf, n)
    unsafe_read(http, pointer(buf.data, buf.size + 1), n)
    buf.size += n
    n # added line
end

Also, I don't know whether this function is used safely internally in HTTP, but for a given IOBuffer which is not empty, this will segfault easily, as Base.ensureroom takes only buf.ptr into account, but only buf.size is updated in the above readbytes! method, e.g.

using HTTP
buf = IOBuffer()
HTTP.open("GET", "http://httpbin.org/stream/10") do io
    while !eof(io)
        k = bytesavailable(io)
        n = readbytes!(io, buf, k)
        @show k, n, length(buf.data), buf.size, buf.ptr
    end
end

gives

(k, n, length(buf.data), buf.size, buf.ptr) = (516, 774, 516, 774, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (516, 1290, 516, 1290, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (258, 1548, 516, 1548, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (516, 2064, 516, 2064, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (258, 2322, 516, 2322, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (258, 2580, 516, 2580, 1)
(k, n, length(buf.data), buf.size, buf.ptr) = (0, 2580, 516, 2580, 1)

signal (11): Segmentation fault
in expression starting at none:0
[...]
@quinnj
Copy link
Member

quinnj commented Aug 30, 2022

You're correct that this is more of an internally used definition, the only usage being:

function Base.read(http::Stream)
    buf = PipeBuffer()
    if ntoread(http) == unknown_length
        while !eof(http)
            readbytes!(http, buf)
        end
    else
        while !eof(http)
            readbytes!(http, buf, ntoread(http))
        end
    end
    return take!(buf)
end

I'd take a PR cleaning this up though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants