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.

Sign up for GitHub

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

Jump to bottom

readavailable blocks, but readbytes! doesn't #89

Closed
quinnj opened this issue Dec 10, 2016 · 1 comment
Closed

readavailable blocks, but readbytes! doesn't #89

quinnj opened this issue Dec 10, 2016 · 1 comment

Comments

@quinnj
Copy link
Member

quinnj commented Dec 10, 2016

No description provided.

@samoconnor
Copy link
Contributor

It looks to me as if readbytes! will block in the current code: https://github.com/JuliaWeb/MbedTLS.jl/blob/master/src/ssl.jl#L238
readbytes! calls unsafe_read and that has a loop while nread < nbytes.

However, the spec for readbytes! says that it is non-blocking:

help?> readbytes!
search: readbytes!

  readbytes!(stream::IO, b::AbstractVector{UInt8}, nb=length(b))

  Read at most nb bytes from stream into b, returning the number of bytes read.
       ^^^^^^^

But, for TCPSocket, readbytes! is blocking: https://github.com/JuliaLang/julia/blob/master/base/stream.jl#L696

So it is probably best for now that MbedTLS.jl follows the TCPSocket behaviour.

This all needs to be sorted out at some point per: JuliaLang/julia#24526

So, unless this is causing a current problem, should we close this issue @quinnj ?
(I'm thinking that part of sorting out #24526 will be making an AbstractIO validation tool, at which point we should test MbedTLS against that)

@quinnj quinnj closed this as completed Apr 3, 2018
samoconnor added a commit that referenced this issue Oct 10, 2018
@samoconnor
Resolve #174 - Step 3. seperate implementation/interface for read/write
0c463a7 
Split the implementation of the Base.unsafe_read and Base.unsafe_write
functions into private ssl_unsafe_read and ssl_unsafe_write.

 - The `Base.unsafe_read`, `Base.readbytes!` and `Base.readavailable`
   methods are now all defined in terms of ssl_unsafe_read.
   The Base.*read* methods no longer have any knowlege of MbedTLS
   internalls, flags etc.

 - `ssl_unsafe_read` is always non-blocking and does not throw
   `EOFError`. Blocking and `EOFError` are now implemented generically
    in the `Base.unsafe_read`, `Base.readbytes!` methods.

 - `ssl_unsafe_read` now propperly handles the `MBEDTLS_ERR_NET_CONN_RESET`
   return code from `f_recv` that was added in
cb889f9.

 - `Base.readavailable` now allocats a buffer with size
   `MBEDTLS_SSL_MAX_CONTENT_LEN` to avoid the need to query
   `bytesavailable()` before reading and to allow for data that arrives
   while `ssl_unsafe_read` is in progress to be included in the result.

 - `Base.readbytes!` no longer shrinks the buffer after calling
   unsafe_read. The spec says "The size of b will be increased if needed,
   but it will never be decreased." Neither the old implementation or
   the new one had provision for increasing the buffer size, but the
   new implementaiton has a precondition check for nbytes <= length(buf).
   The new implementation also adds the `all=true` kw option per the spec
   for `readbytes!(::IOStream`. Behaviour of blocking by default is
   retained.
   (Note: the spec in Base is not self-consistent in this regard because
    the `readbytes!(::IO` spec contradicts the `readbytes!(::IOStream` spec.
    See #89.)

 - Added `ssl_abandon` function to be called when a C function return
   code calls says "you must stop using the SSL context for reading or
   writing".

 - Added `isreadable` and `iswritable` preconditions to the
    `ssl_unsafe_read` and `ssl_unsafe_write` functions.
@samoconnor samoconnor mentioned this issue Oct 10, 2018
Merged
samoconnor added a commit that referenced this issue Oct 10, 2018
@samoconnor
Resolve #174 - Step 3. seperate implementation/interface for read/write
4e59a65 
Split the implementation of the Base.unsafe_read and Base.unsafe_write
functions into private ssl_unsafe_read and ssl_unsafe_write.

 - The `Base.unsafe_read`, `Base.readbytes!` and `Base.readavailable`
   methods are now all defined in terms of ssl_unsafe_read.
   The Base.*read* methods no longer have any knowlege of MbedTLS
   internalls, flags etc.

 - `ssl_unsafe_read` is always non-blocking and does not throw
   `EOFError`. Blocking and `EOFError` are now implemented generically
    in the `Base.unsafe_read`, `Base.readbytes!` methods.

 - `ssl_unsafe_read` now propperly handles the `MBEDTLS_ERR_NET_CONN_RESET`
   return code from `f_recv` that was added in
cb889f9.

 - `Base.readavailable` now allocats a buffer with size
   `MBEDTLS_SSL_MAX_CONTENT_LEN` to avoid the need to query
   `bytesavailable()` before reading and to allow for data that arrives
   while `ssl_unsafe_read` is in progress to be included in the result.

 - `Base.readbytes!` no longer shrinks the buffer after calling
   unsafe_read. The spec says "The size of b will be increased if needed,
   but it will never be decreased." Neither the old implementation or
   the new one had provision for increasing the buffer size, but the
   new implementaiton has a precondition check for nbytes <= length(buf).
   The new implementation also adds the `all=true` kw option per the spec
   for `readbytes!(::IOStream`. Behaviour of blocking by default is
   retained.
   (Note: the spec in Base is not self-consistent in this regard because
    the `readbytes!(::IO` spec contradicts the `readbytes!(::IOStream` spec.
    See #89.)

 - Added `ssl_abandon` function to be called when a C function return
   code calls says "you must stop using the SSL context for reading or
   writing".

 - Added `isreadable` and `iswritable` preconditions to the
    `ssl_unsafe_read` and `ssl_unsafe_write` functions.

 - Throw EOFError if socket closes during handshake to avoid looping forever
samoconnor added a commit that referenced this issue Oct 12, 2018
@samoconnor
Resolve #174 - Step 3. seperate implementation/interface for read/write
458c8a4 
Split the implementation of the Base.unsafe_read and Base.unsafe_write
functions into private ssl_unsafe_read and ssl_unsafe_write.

 - The `Base.unsafe_read`, `Base.readbytes!` and `Base.readavailable`
   methods are now all defined in terms of ssl_unsafe_read.
   The Base.*read* methods no longer have any knowlege of MbedTLS
   internalls, flags etc.

 - `ssl_unsafe_read` is always non-blocking and does not throw
   `EOFError`. Blocking and `EOFError` are now implemented generically
    in the `Base.unsafe_read`, `Base.readbytes!` methods.

 - `ssl_unsafe_read` now propperly handles the `MBEDTLS_ERR_NET_CONN_RESET`
   return code from `f_recv` that was added in
cb889f9.

 - `Base.readavailable` now allocats a buffer with size
   `MBEDTLS_SSL_MAX_CONTENT_LEN` to avoid the need to query
   `bytesavailable()` before reading and to allow for data that arrives
   while `ssl_unsafe_read` is in progress to be included in the result.

 - `Base.readbytes!` no longer shrinks the buffer after calling
   unsafe_read. The spec says "The size of b will be increased if needed,
   but it will never be decreased." Neither the old implementation or
   the new one had provision for increasing the buffer size, but the
   new implementaiton has a precondition check for nbytes <= length(buf).
   The new implementation also adds the `all=true` kw option per the spec
   for `readbytes!(::IOStream`. Behaviour of blocking by default is
   retained.
   (Note: the spec in Base is not self-consistent in this regard because
    the `readbytes!(::IO` spec contradicts the `readbytes!(::IOStream` spec.
    See #89.)

 - Added `ssl_abandon` function to be called when a C function return
   code calls says "you must stop using the SSL context for reading or
   writing".

 - Added `isreadable` and `iswritable` preconditions to the
    `ssl_unsafe_read` and `ssl_unsafe_write` functions.

 - Throw EOFError if socket closes during handshake to avoid looping forever
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@quinnj @samoconnor