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

Use Base.IOError more consistently for read/write operation failures #257

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Use Base.IOError more consistently for read/write operation failures #257

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 23 additions & 35 deletions src/ssl.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,23 +165,6 @@ else
Base.nb_available(ctx::SSLContext) = ctx.bytesavailable
end

"""
eof(ctx::SSLContext)

True if not `isreadable` and there are no more `bytesavailable` to read.
"""
function Base.eof(ctx::SSLContext)
ctx.bytesavailable > 0 && return false
wait_for_decrypted_data(ctx)
# note that the following are racy when there are multiple concurrent
# users of an `SSLContext`, but we're at least not going to return
# true until ctx.isreadable is false, which means we received a
# close_notify, the underlying connection was closed, or some
# other fatal ssl error occurred
ctx.bytesavailable > 0 && return false
return !ctx.isreadable
end

"""
close(ctx::SSLContext)

Expand Down Expand Up @@ -228,9 +211,6 @@ function closewrite(ctx::SSLContext)
nothing
end




# Sending Data

"""
Expand All @@ -242,7 +222,7 @@ function which sends it to the underlying connection (`ctx.bio`).
See `f_send` and `set_bio!` below.
"""
function ssl_unsafe_write(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt)
ctx.close_notify_sent && throw(ArgumentError("`unsafe_write` requires `!ctx.close_notify_sent`"))
ctx.close_notify_sent && throw(Base.IOError("`unsafe_write` requires `!ctx.close_notify_sent`", 0))

nwritten = 0 ;@🤖 "ssl_write ➡️ $nbytes"
while nwritten < nbytes
Expand All @@ -254,7 +234,7 @@ function ssl_unsafe_write(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt)
continue
elseif n < 0
ssl_abandon(ctx) ;@🤖 "ssl_write 💥"
throw(MbedException(n))
throw(Base.IOError(strerror(n), n))
end
nwritten += n
end
Expand Down Expand Up @@ -295,14 +275,18 @@ end
# Receiving Data

"""
While there are no decrypted bytes available but the connection is readable:
- If the TLS buffer has no pending (unprocessed) data wait for
more encrypted data to arrive on the underlying connection.
- Run a zero-byte read to allow the library to process its internal buffer,
and/or read from the underlying connection.
- `ssl_unsafe_read` updates the `isreadable` and `bytesavailable` state.
eof(ctx::SSLContext)

True if not `isreadable` and there are no more `bytesavailable` to read.
"""
function wait_for_decrypted_data(ctx)
function Base.eof(ctx::SSLContext)
ctx.bytesavailable > 0 && return false
# While there are no decrypted bytes available but the connection is readable:
# - If the TLS buffer has no pending (unprocessed) data, wait for
# more encrypted data to arrive on the underlying connection.
# - Run a zero-byte read to allow the library to process its internal buffer,
# and/or read from the underlying connection.
# - `ssl_unsafe_read` updates the `isreadable` and `bytesavailable` state.
lock(ctx.waitlock)
try
while ctx.isreadable && ctx.bytesavailable <= 0
Expand All @@ -314,9 +298,15 @@ function wait_for_decrypted_data(ctx)
finally
unlock(ctx.waitlock)
end
# note that the following are racy when there are multiple concurrent
# users of an `SSLContext`, but we're at least not going to return
# true until ctx.isreadable is false, which means we received a
# close_notify, the underlying connection was closed, or some
# other fatal ssl error occurred
ctx.bytesavailable > 0 && return false
return !ctx.isreadable
end


"""
ssl_unsafe_read(::SSLContext, buf, nbytes)

Expand All @@ -342,8 +332,7 @@ When an unhandled exception occurs `isreadable` is set to false.
"""
function ssl_unsafe_read(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt)

ctx.isreadable ||
throw(ArgumentError("`ssl_unsafe_read` requires `isreadable(::SSLContext)`"))
ctx.isreadable || throw(Base.IOError("`ssl_unsafe_read` requires `isreadable(::SSLContext)`", 0))

nread::UInt = 0
try
Expand Down Expand Up @@ -372,8 +361,7 @@ function ssl_unsafe_read(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt)
ctx.bytesavailable = 0 ;@😬 "ssl_read ⌛️ $nread"
return nread
elseif n < 0
ssl_abandon(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it intentional to drop the call to ssl_abandon here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm, good question. I think the original intent was just to throw an IOError instead of an MbedException, but we perhaps want the other cleanup that happens in ssl_abandon, so maybe it would be better if we changed ssl_abandon to just throw the IOError instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup sounds like the right thing to do. I guess the scenario where it would make any difference would be to give a more clear error message were someone to catch the IO-error and then try to read or write again?

I wouldn't expect ssl_abandon to throw though. It would make it harder for me to reason about the control flow in ssl_unsafe_read.

A helper like throw(mbed_ioerr(n)) could come in handy after calling ssl_abandon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6633354

throw(MbedException(n))
throw(Base.IOError(strerror(n), n))
end

nread += n
Expand All @@ -384,7 +372,7 @@ function ssl_unsafe_read(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt)
return nread
end
end
catch e ;@💀 "ssl_read 💥"
catch ;@💀 "ssl_read 💥"
ssl_abandon(ctx)
rethrow()
end
Expand Down