From bf9b1774c99fb7e2336867274cabc62edb045129 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Wed, 2 Oct 2024 23:55:54 +0200 Subject: [PATCH 01/11] Minor cleanups - Fix callback formatting - Replace all uses of `@async` with `Threads.@spawn` --- src/callbacks.jl | 102 +++++++++++++++++++++++------------------------ src/channel.jl | 2 +- src/server.jl | 2 +- 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/src/callbacks.jl b/src/callbacks.jl index f9bb136..8a9dd72 100644 --- a/src/callbacks.jl +++ b/src/callbacks.jl @@ -376,77 +376,77 @@ function Base.setproperty!(self::ChannelCallbacks, name::Symbol, value) # Why do some of these callbacks use 1 for denied and some -1? Who knows ¯\_(ツ)_/¯ if name === :on_data - ptr.channel_data_function = @_gencb(:channel_data, value, - Int, 0, Cint, - Cint, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid}, Cuint, Cint, Ptr{Cvoid})) + ptr.channel_data_function = @_gencb(:channel_data, value, + Int, 0, Cint, + Cint, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid}, Cuint, Cint, Ptr{Cvoid})) elseif name === :on_eof - ptr.channel_eof_function = @_gencb(:channel_eof, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) + ptr.channel_eof_function = @_gencb(:channel_eof, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) elseif name === :on_close - ptr.channel_close_function = @_gencb(:channel_close, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) + ptr.channel_close_function = @_gencb(:channel_close, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) elseif name === :on_signal - ptr.channel_signal_function = @_gencb(:channel_signal, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) + ptr.channel_signal_function = @_gencb(:channel_signal, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) elseif name === :on_exit_status - ptr.channel_exit_status_function = @_gencb(:channel_exit_status, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Cint, Ptr{Cvoid})) + ptr.channel_exit_status_function = @_gencb(:channel_exit_status, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Cint, Ptr{Cvoid})) elseif name === :on_exit_signal - ptr.channel_exit_signal_function = @_gencb(:channel_exit_signal, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Cstring, Cint, Cstring, Cstring, Ptr{Cvoid})) + ptr.channel_exit_signal_function = @_gencb(:channel_exit_signal, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Cstring, Cint, Cstring, Cstring, Ptr{Cvoid})) elseif name === :on_pty_request - ptr.channel_pty_request_function = @_gencb(:channel_pty_request, value, - Bool, false, ret -> Cint(ret ? 0 : -1), - Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Cint, Cint, Cint, Cint, Ptr{Cvoid})) + ptr.channel_pty_request_function = @_gencb(:channel_pty_request, value, + Bool, false, ret -> Cint(ret ? 0 : -1), + Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Cint, Cint, Cint, Cint, Ptr{Cvoid})) elseif name === :on_shell_request - ptr.channel_shell_request_function = @_gencb(:channel_shell_request, value, - Bool, false, ret -> Cint(ret ? 0 : 1), - Cint, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) + ptr.channel_shell_request_function = @_gencb(:channel_shell_request, value, + Bool, false, ret -> Cint(ret ? 0 : 1), + Cint, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) elseif name === :on_auth_agent_req - ptr.channel_auth_agent_req_function = @_gencb(:channel_auth_agent_req, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) + ptr.channel_auth_agent_req_function = @_gencb(:channel_auth_agent_req, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) elseif name === :on_x11_req - ptr.channel_x11_req_function = @_gencb(:channel_x11_req, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Cint, Cstring, Cstring, Cuint, Ptr{Cvoid})) + ptr.channel_x11_req_function = @_gencb(:channel_x11_req, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Cint, Cstring, Cstring, Cuint, Ptr{Cvoid})) elseif name === :on_pty_window_change - ptr.channel_pty_window_change_function = @_gencb(:channel_pty_window_change, value, - Bool, false, ret -> Cint(ret ? 0 : -1), - Cint, (lib.ssh_session, lib.ssh_channel, Cint, Cint, Cint, Cint, Ptr{Cvoid})) + ptr.channel_pty_window_change_function = @_gencb(:channel_pty_window_change, value, + Bool, false, ret -> Cint(ret ? 0 : -1), + Cint, (lib.ssh_session, lib.ssh_channel, Cint, Cint, Cint, Cint, Ptr{Cvoid})) elseif name === :on_exec_request - ptr.channel_exec_request_function = @_gencb(:channel_exec_request, value, - Bool, false, ret -> Cint(ret ? 0 : 1), - Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) + ptr.channel_exec_request_function = @_gencb(:channel_exec_request, value, + Bool, false, ret -> Cint(ret ? 0 : 1), + Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) elseif name === :on_env_request - ptr.channel_env_request_function = @_gencb(:channel_env_request, value, - Bool, false, ret -> Cint(ret ? 0 : 1), - Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Cstring, Ptr{Cvoid})) + ptr.channel_env_request_function = @_gencb(:channel_env_request, value, + Bool, false, ret -> Cint(ret ? 0 : 1), + Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Cstring, Ptr{Cvoid})) elseif name === :on_subsystem_request - ptr.channel_subsystem_request_function = @_gencb(:channel_subsystem_request, value, - Bool, false, ret -> Cint(ret ? 0 : 1), - Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) + ptr.channel_subsystem_request_function = @_gencb(:channel_subsystem_request, value, + Bool, false, ret -> Cint(ret ? 0 : 1), + Cint, (lib.ssh_session, lib.ssh_channel, Cstring, Ptr{Cvoid})) elseif name === :on_write_wontblock - ptr.channel_write_wontblock_function = @_gencb(:channel_write_wontblock, value, - Int, 0, Cint, - Cint, (lib.ssh_session, lib.ssh_channel, Cuint, Ptr{Cvoid})) + ptr.channel_write_wontblock_function = @_gencb(:channel_write_wontblock, value, + Int, 0, Cint, + Cint, (lib.ssh_session, lib.ssh_channel, Cuint, Ptr{Cvoid})) elseif name === :on_open_response - ptr.channel_open_response_function = @_gencb(:channel_open_response, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Bool, Ptr{Cvoid})) + ptr.channel_open_response_function = @_gencb(:channel_open_response, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Bool, Ptr{Cvoid})) elseif name === :on_request_response - ptr.channel_request_response_function = @_gencb(:channel_request_response, value, - Nothing, nothing, identity, - Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) + ptr.channel_request_response_function = @_gencb(:channel_request_response, value, + Nothing, nothing, identity, + Cvoid, (lib.ssh_session, lib.ssh_channel, Ptr{Cvoid})) else setfield!(self, name, value) end diff --git a/src/channel.jl b/src/channel.jl index 73c2b0b..6ef0aa4 100644 --- a/src/channel.jl +++ b/src/channel.jl @@ -54,7 +54,7 @@ function _finalizer(sshchan::SshChannel) try close(sshchan) catch ex - # Note the use of @async to avoid a task switch, which is forbidden in a + # Note the use of @spawn to avoid a task switch, which is forbidden in a # finalizer. Threads.@spawn @error "Caught exception while finalizing SshChannel" exception=(ex, catch_backtrace()) end diff --git a/src/server.jl b/src/server.jl index 2da69ae..8f67f1b 100644 --- a/src/server.jl +++ b/src/server.jl @@ -857,7 +857,7 @@ function DemoServer(f::Function, args...; timeout=10, kill_timeout=3, kwargs...) # If the function is still running, we attempt to kill it explicitly kill_failed = nothing if still_running - @async Base.throwto(t, InterruptException()) + Threads.@spawn Base.throwto(t, InterruptException()) result = timedwait(() -> istaskdone(t), kill_timeout) kill_failed = result == :timed_out end From 387c51ad77338a84df1ae70f48cbbf691b0d1391 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Fri, 4 Oct 2024 00:11:20 +0200 Subject: [PATCH 02/11] Fix race condition in the server Previously we were closing all the client operations while the client handler task was still running, which could lead to extra messages being processed while channels etc were being shut down (which could lead to segfaults). --- docs/src/changelog.md | 2 ++ src/server.jl | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/src/changelog.md b/docs/src/changelog.md index c2f114b..904012d 100644 --- a/docs/src/changelog.md +++ b/docs/src/changelog.md @@ -23,6 +23,8 @@ Changelog](https://keepachangelog.com). [`Session`](@ref) is disconnected by the remote end ([#13]). - Fixed some concurrency bugs in the [`Demo.DemoServer`](@ref) and [`SessionEvent`](@ref) ([#15]). +- Fixed a race condition in the [`Demo.DemoServer`](@ref) that could cause + segfaults ([#16]). ## [v0.5.0] - 2024-08-10 diff --git a/src/server.jl b/src/server.jl index 8f67f1b..9e30440 100644 --- a/src/server.jl +++ b/src/server.jl @@ -711,6 +711,9 @@ end end function Base.close(client::Client) + close(client.session_event) + wait(client.task) + for op in client.channel_operations close(op) end @@ -719,8 +722,6 @@ function Base.close(client::Client) close(sshchan) end - close(client.session_event) - wait(client.task) close(client.session) end From 3c14bab771fb8975ca7b5c748e5116b5aef5f0dd Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:25:55 +0200 Subject: [PATCH 03/11] Add support for generating wrappers for blocking libssh calls This is required for SFTP support. --- gen/gen.jl | 164 +++++++++++++++++++++++++++--------- src/bindings.jl | 220 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 305 insertions(+), 79 deletions(-) diff --git a/gen/gen.jl b/gen/gen.jl index 67715c2..4e7d33f 100644 --- a/gen/gen.jl +++ b/gen/gen.jl @@ -4,7 +4,7 @@ import XML import MacroTools import MacroTools: @capture import Clang -import Clang.Generators: ExprNode, AbstractFunctionNodeType +import Clang.Generators: ExprNode, AbstractFunctionNodeType, FunctionProto include("../doc_utils.jl") import .DocUtils: read_tags, get_url @@ -15,12 +15,18 @@ ctx_objects = Dict{Symbol, Any}() # These are lists of functions that we'll rewrite to return Julia types string_functions = [:ssh_message_auth_user, :ssh_message_auth_password, :ssh_userauth_kbdint_getname, :ssh_userauth_kbdint_getanswer, - :ssh_userauth_kbdint_getprompt] + :ssh_userauth_kbdint_getprompt, + :sftp_extensions_get_name, :sftp_extensions_get_data] bool_functions = [:ssh_message_auth_kbdint_is_response] ssh_ok_functions = [:ssh_message_auth_reply_success, :ssh_message_auth_set_methods, :ssh_message_reply_default, :ssh_options_get, :ssh_options_set, :ssh_options_get_port] -all_rewritable_functions = vcat(string_functions, bool_functions, ssh_ok_functions) + +# These functions require the ssh_session to be in blocking mode, so we always +# call them with @threadcall. +threadcall_functions = [:sftp_new, :sftp_init, :sftp_open, :sftp_close, + :sftp_home_directory, :sftp_stat, :sftp_aio_wait_read] +all_rewritable_functions = vcat(string_functions, bool_functions, ssh_ok_functions, threadcall_functions) """ Helper function to generate documentation for symbols with missing docstrings. @@ -60,12 +66,13 @@ function get_docs(node::ExprNode, doc::Vector{String}) elseif node.id == :sftp_limits_t String["Pointer to a [`sftp_limits_struct`](@ref)"] - # Internal Clang.jl structs start with '__' and we don't want to document them - elseif startswith(string(node.id), "__") + # Internal Clang.jl structs and helper functions from us start with '_' and + # we don't want to document them. + elseif startswith(string(node.id), "_") String[] elseif node.id in all_rewritable_functions - symbol_ref = isempty(doc) && haskey(tags, node.id) ? "[`$(node.id)`]($url)" : "`$(node.id)`" + symbol_ref = isempty(doc) && haskey(tags, node.id) ? "[`$(node.id)()`]($url)" : "`$(node.id)()`" original_docs_mention = isempty(doc) ? "" : " Original upstream documentation is below." autogen_line = String["Auto-generated wrapper around $symbol_ref.$original_docs_mention"] @@ -85,60 +92,141 @@ function get_docs(node::ExprNode, doc::Vector{String}) end end +function rewrite_string_function(name, args, body) + quote + function $name($(args...); throw=true) + ret = $body + + if ret == C_NULL + if throw + Base.throw(LibSSHException($("Error from $name, no string found (returned C_NULL)"))) + else + return ret + end + end + + return unsafe_string(Ptr{UInt8}(ret)) + end + end +end + +function rewrite_bool_function(name, args, body) + quote + function $name($(args...)) + ret = $body + return Bool(ret) + end + end +end + +function rewrite_ssh_ok_function(name, args, body) + quote + function $name($(args...); throw=true) + ret = $body + + if ret != SSH_OK && throw + # This ugly concatenation is necessary because we + # have to interpolate the function name into the + # error string but also keep the return value + # interpolation from being escaped. + Base.throw(LibSSHException($("Error from $name, did not return SSH_OK: ") * "$(ret)")) + end + + return ret + end + end +end + +""" +Rewrites a blocking libssh function into one that uses @threadcall. Some parts +of the libssh API don't support the non-blocking API, so to avoid these calls +blocking a whole thread we call them with @threadcall. + +The problem is that @threadcall will not mark the calling region as GC safe, +which means that if the @threadcall returning depends on some other Julia code +executing (i.e. with the DemoServer) we'll likely deadlock when the other Julia +code tries to allocate and hits the GC. + +To get around this we rewrite the original wrapper to call a @cfunction that +will create a safe region around the @ccall. In some future Julia release this +will be unnecessary and we'll be able to directly @threadcall blocking +functions: https://github.com/JuliaLang/julia/pull/55956 +""" +function rewrite_threadcall_function!(dag, node, name, args, body) + if !@capture(body, @ccall ccall_func_(ccall_args__)::ccall_ret_) + error("Couldn't parse @ccall expr") + end + + # We require the exact argument types to be passed because @ccall's + # automatic type conversion may allocate. It's easier to just require the + # user to do the right thing. + arg_types = [expr.args[2] for expr in ccall_args] + + cfunc_name = Symbol(:_threadcall_, name) + cfunc_expr = quote + function $cfunc_name($(ccall_args...)) + gc_state = @ccall jl_gc_safe_enter()::Int8 + ret = $body + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + + return ret + end + end + cfunc_expr = MacroTools.prettify(cfunc_expr) + + new_node = ExprNode(cfunc_name, FunctionProto(), node.cursor, [cfunc_expr], Int[]) + + wrapper = quote + function $name($(ccall_args...)) + cfunc = @cfunction($cfunc_name, $ccall_ret, ($(arg_types...),)) + return @threadcall(cfunc, $ccall_ret, ($(arg_types...),), $(args...)) + end + end + + return wrapper, new_node +end + function rewrite!(ctx) dag = ctx.dag - for node in dag.nodes + nodes_to_insert = [] + for node_idx in eachindex(dag.nodes) + node = dag.nodes[node_idx] + for i in eachindex(node.exprs) expr = node.exprs[i] # Look for function expressions if @capture(expr, function name_(args__) body_ end) wrapper = nothing + name_str = string(name) # Check if we can rewrite the function if name in string_functions - wrapper = quote - if ret == C_NULL - if throw - Base.throw(LibSSHException($("Error from $name, no string found (returned C_NULL)"))) - else - return ret - end - end - - return unsafe_string(Ptr{UInt8}(ret)) - end + wrapper = rewrite_string_function(name, args, body) elseif name in bool_functions - wrapper = :(return Bool(ret)) + wrapper = rewrite_bool_function(name, args, body) elseif name in ssh_ok_functions - wrapper = quote - if ret != SSH_OK && throw - # This ugly concatenation is necessary because we - # have to interpolate the function name into the - # error string but also keep the return value - # interpolation from being escaped. - Base.throw(LibSSHException($("Error from $name, did not return SSH_OK: ") * "$(ret)")) - end - - return ret - end + wrapper = rewrite_ssh_ok_function(name, args, body) + elseif name in threadcall_functions + wrapper, new_node = rewrite_threadcall_function!(dag, node, name, args, body) + + push!(nodes_to_insert, (node_idx, new_node)) end if !isnothing(wrapper) - new_expr = quote - function $name($(args...); throw=true) - ret = $body - $wrapper - end - end - # Note that the node retains the old ID, only the expression changed - node.exprs[i] = MacroTools.prettify(new_expr) + node.exprs[i] = MacroTools.prettify(wrapper) end end end end + + # Iterate over the nodes to insert in reverse so we don't invalidate their + # indices. + for (idx, node) in reverse(nodes_to_insert) + insert!(dag.nodes, idx, node) + end end cd(@__DIR__) do diff --git a/src/bindings.jl b/src/bindings.jl index 4eb3bf2..183e8f9 100644 --- a/src/bindings.jl +++ b/src/bindings.jl @@ -1426,7 +1426,7 @@ end """ ssh_options_set(session, type, value; throw = true) -Auto-generated wrapper around [`ssh_options_set`](https://api.libssh.org/stable/group__libssh__session.html#ga7a801b85800baa3f4e16f5b47db0a73d). +Auto-generated wrapper around [`ssh_options_set()`](https://api.libssh.org/stable/group__libssh__session.html#ga7a801b85800baa3f4e16f5b47db0a73d). """ function ssh_options_set(session, type, value; throw = true) ret = @ccall(libssh.ssh_options_set(session::ssh_session, type::ssh_options_e, value::Ptr{Cvoid})::Cint) @@ -1439,7 +1439,7 @@ end """ ssh_options_get(session, type, value; throw = true) -Auto-generated wrapper around [`ssh_options_get`](https://api.libssh.org/stable/group__libssh__session.html#gaaa9d400920cad4d6e4a0fb09ff8c7b01). +Auto-generated wrapper around [`ssh_options_get()`](https://api.libssh.org/stable/group__libssh__session.html#gaaa9d400920cad4d6e4a0fb09ff8c7b01). """ function ssh_options_get(session, type, value; throw = true) ret = @ccall(libssh.ssh_options_get(session::ssh_session, type::ssh_options_e, value::Ptr{Ptr{Cchar}})::Cint) @@ -1452,7 +1452,7 @@ end """ ssh_options_get_port(session, port_target; throw = true) -Auto-generated wrapper around [`ssh_options_get_port`](https://api.libssh.org/stable/group__libssh__session.html#gaa298d8445355420d80f2d968477ba86f). +Auto-generated wrapper around [`ssh_options_get_port()`](https://api.libssh.org/stable/group__libssh__session.html#gaa298d8445355420d80f2d968477ba86f). """ function ssh_options_get_port(session, port_target; throw = true) ret = @ccall(libssh.ssh_options_get_port(session::ssh_session, port_target::Ptr{Cuint})::Cint) @@ -1943,7 +1943,7 @@ end """ ssh_userauth_kbdint_getname(session; throw = true) -Auto-generated wrapper around [`ssh_userauth_kbdint_getname`](https://api.libssh.org/stable/group__libssh__auth.html#ga5d6f5eb0ed09fe2c7a2ac69b972e130e). +Auto-generated wrapper around [`ssh_userauth_kbdint_getname()`](https://api.libssh.org/stable/group__libssh__auth.html#ga5d6f5eb0ed09fe2c7a2ac69b972e130e). """ function ssh_userauth_kbdint_getname(session; throw = true) ret = @ccall(libssh.ssh_userauth_kbdint_getname(session::ssh_session)::Ptr{Cchar}) @@ -1969,7 +1969,7 @@ end """ ssh_userauth_kbdint_getprompt(session, i, echo; throw = true) -Auto-generated wrapper around [`ssh_userauth_kbdint_getprompt`](https://api.libssh.org/stable/group__libssh__auth.html#ga15c0f954f79d73e1ac5981ac483efb75). +Auto-generated wrapper around [`ssh_userauth_kbdint_getprompt()`](https://api.libssh.org/stable/group__libssh__auth.html#ga15c0f954f79d73e1ac5981ac483efb75). """ function ssh_userauth_kbdint_getprompt(session, i, echo; throw = true) ret = @ccall(libssh.ssh_userauth_kbdint_getprompt(session::ssh_session, i::Cuint, echo::Ptr{Cchar})::Ptr{Cchar}) @@ -1995,7 +1995,7 @@ end """ ssh_userauth_kbdint_getanswer(session, i; throw = true) -Auto-generated wrapper around [`ssh_userauth_kbdint_getanswer`](https://api.libssh.org/stable/group__libssh__auth.html#ga4f55ed8bc6f553423ab1c92598d0194b). +Auto-generated wrapper around [`ssh_userauth_kbdint_getanswer()`](https://api.libssh.org/stable/group__libssh__auth.html#ga4f55ed8bc6f553423ab1c92598d0194b). """ function ssh_userauth_kbdint_getanswer(session, i; throw = true) ret = @ccall(libssh.ssh_userauth_kbdint_getanswer(session::ssh_session, i::Cuint)::Ptr{Cchar}) @@ -2645,6 +2645,36 @@ struct sftp_attributes_struct extended_type::ssh_string extended_data::ssh_string end +function Base.getproperty(x::Ptr{sftp_attributes_struct}, f::Symbol) + f === :name && return Ptr{Ptr{Cchar}}(x + 0) + f === :longname && return Ptr{Ptr{Cchar}}(x + 8) + f === :flags && return Ptr{UInt32}(x + 16) + f === :type && return Ptr{UInt8}(x + 20) + f === :size && return Ptr{UInt64}(x + 24) + f === :uid && return Ptr{UInt32}(x + 32) + f === :gid && return Ptr{UInt32}(x + 36) + f === :owner && return Ptr{Ptr{Cchar}}(x + 40) + f === :group && return Ptr{Ptr{Cchar}}(x + 48) + f === :permissions && return Ptr{UInt32}(x + 56) + f === :atime64 && return Ptr{UInt64}(x + 64) + f === :atime && return Ptr{UInt32}(x + 72) + f === :atime_nseconds && return Ptr{UInt32}(x + 76) + f === :createtime && return Ptr{UInt64}(x + 80) + f === :createtime_nseconds && return Ptr{UInt32}(x + 88) + f === :mtime64 && return Ptr{UInt64}(x + 96) + f === :mtime && return Ptr{UInt32}(x + 104) + f === :mtime_nseconds && return Ptr{UInt32}(x + 108) + f === :acl && return Ptr{ssh_string}(x + 112) + f === :extended_count && return Ptr{UInt32}(x + 120) + f === :extended_type && return Ptr{ssh_string}(x + 128) + f === :extended_data && return Ptr{ssh_string}(x + 136) + return getfield(x, f) +end + +function Base.setproperty!(x::Ptr{sftp_attributes_struct}, f::Symbol, v) + unsafe_store!(getproperty(x, f), v) +end + const sftp_attributes = Ptr{sftp_attributes_struct} @@ -2822,8 +2852,19 @@ Base.unsafe_convert(::Type{Ptr{__JL_sftp_request_queue_struct}}, x::Base.RefValu Base.unsafe_convert(::Type{Ptr{__JL_sftp_request_queue_struct}}, x::Ptr{sftp_request_queue_struct}) = Ptr{__JL_sftp_request_queue_struct}(x) +function _threadcall_sftp_new(session::ssh_session) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_new(session::ssh_session)::sftp_session) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_new(session) + sftp_new(session::ssh_session) + +Auto-generated wrapper around `sftp_new()`. Original upstream documentation is below. + +--- Creates a new sftp session. @@ -2836,8 +2877,9 @@ A new sftp session or NULL on error. # See also [`sftp_free`](@ref)(), [`sftp_init`](@ref)() """ -function sftp_new(session) - @ccall libssh.sftp_new(session::ssh_session)::sftp_session +function sftp_new(session::ssh_session) + cfunc = @cfunction(_threadcall_sftp_new, sftp_session, (ssh_session,)) + return @threadcall(cfunc, sftp_session, (ssh_session,), session) end """ @@ -2869,8 +2911,19 @@ function sftp_free(sftp) @ccall libssh.sftp_free(sftp::sftp_session)::Cvoid end +function _threadcall_sftp_init(sftp::sftp_session) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_init(sftp::sftp_session)::Cint) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_init(sftp) + sftp_init(sftp::sftp_session) + +Auto-generated wrapper around `sftp_init()`. Original upstream documentation is below. + +--- Initialize the sftp protocol with the server. @@ -2883,8 +2936,9 @@ This function involves the SFTP protocol initialization (as described in the SFT # See also [`sftp_new`](@ref)() """ -function sftp_init(sftp) - @ccall libssh.sftp_init(sftp::sftp_session)::Cint +function sftp_init(sftp::sftp_session) + cfunc = @cfunction(_threadcall_sftp_init, Cint, (sftp_session,)) + return @threadcall(cfunc, Cint, (sftp_session,), sftp) end """ @@ -2920,7 +2974,11 @@ function sftp_extensions_get_count(sftp) end """ - sftp_extensions_get_name(sftp, indexn) + sftp_extensions_get_name(sftp, indexn; throw = true) + +Auto-generated wrapper around `sftp_extensions_get_name()`. Original upstream documentation is below. + +--- Get the name of the extension provided by the server. @@ -2930,12 +2988,24 @@ Get the name of the extension provided by the server. # Returns The name of the extension. """ -function sftp_extensions_get_name(sftp, indexn) - @ccall libssh.sftp_extensions_get_name(sftp::sftp_session, indexn::Cuint)::Ptr{Cchar} +function sftp_extensions_get_name(sftp, indexn; throw = true) + ret = @ccall(libssh.sftp_extensions_get_name(sftp::sftp_session, indexn::Cuint)::Ptr{Cchar}) + if ret == C_NULL + if throw + Base.throw(LibSSHException("Error from sftp_extensions_get_name, no string found (returned C_NULL)")) + else + return ret + end + end + return unsafe_string(Ptr{UInt8}(ret)) end """ - sftp_extensions_get_data(sftp, indexn) + sftp_extensions_get_data(sftp, indexn; throw = true) + +Auto-generated wrapper around `sftp_extensions_get_data()`. Original upstream documentation is below. + +--- Get the data of the extension provided by the server. @@ -2947,8 +3017,16 @@ This is normally the version number of the extension. # Returns The data of the extension. """ -function sftp_extensions_get_data(sftp, indexn) - @ccall libssh.sftp_extensions_get_data(sftp::sftp_session, indexn::Cuint)::Ptr{Cchar} +function sftp_extensions_get_data(sftp, indexn; throw = true) + ret = @ccall(libssh.sftp_extensions_get_data(sftp::sftp_session, indexn::Cuint)::Ptr{Cchar}) + if ret == C_NULL + if throw + Base.throw(LibSSHException("Error from sftp_extensions_get_data, no string found (returned C_NULL)")) + else + return ret + end + end + return unsafe_string(Ptr{UInt8}(ret)) end """ @@ -3023,8 +3101,19 @@ function sftp_dir_eof(dir) @ccall libssh.sftp_dir_eof(dir::sftp_dir)::Cint end +function _threadcall_sftp_stat(session::sftp_session, path::Ptr{Cchar}) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_stat(session::sftp_session, path::Ptr{Cchar})::sftp_attributes) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_stat(session, path) + sftp_stat(session::sftp_session, path::Ptr{Cchar}) + +Auto-generated wrapper around `sftp_stat()`. Original upstream documentation is below. + +--- Get information about a file or directory. @@ -3036,8 +3125,9 @@ The sftp attributes structure of the file or directory, NULL on error with ssh a # See also [`sftp_get_error`](@ref)() """ -function sftp_stat(session, path) - @ccall libssh.sftp_stat(session::sftp_session, path::Ptr{Cchar})::sftp_attributes +function sftp_stat(session::sftp_session, path::Ptr{Cchar}) + cfunc = @cfunction(_threadcall_sftp_stat, sftp_attributes, (sftp_session, Ptr{Cchar})) + return @threadcall(cfunc, sftp_attributes, (sftp_session, Ptr{Cchar}), session, path) end """ @@ -3101,8 +3191,19 @@ function sftp_closedir(dir) @ccall libssh.sftp_closedir(dir::sftp_dir)::Cint end +function _threadcall_sftp_close(file::sftp_file) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_close(file::sftp_file)::Cint) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_close(file) + sftp_close(file::sftp_file) + +Auto-generated wrapper around `sftp_close()`. Original upstream documentation is below. + +--- Close an open file handle. @@ -3113,12 +3214,24 @@ Returns SSH\\_NO\\_ERROR or [`SSH_ERROR`](@ref) if an error occurred. # See also [`sftp_open`](@ref)() """ -function sftp_close(file) - @ccall libssh.sftp_close(file::sftp_file)::Cint +function sftp_close(file::sftp_file) + cfunc = @cfunction(_threadcall_sftp_close, Cint, (sftp_file,)) + return @threadcall(cfunc, Cint, (sftp_file,), file) +end + +function _threadcall_sftp_open(session::sftp_session, file::Ptr{Cchar}, accesstype::Cint, mode::mode_t) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_open(session::sftp_session, file::Ptr{Cchar}, accesstype::Cint, mode::mode_t)::sftp_file) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret end """ - sftp_open(session, file, accesstype, mode) + sftp_open(session::sftp_session, file::Ptr{Cchar}, accesstype::Cint, mode::mode_t) + +Auto-generated wrapper around `sftp_open()`. Original upstream documentation is below. + +--- Open a file on the server. @@ -3132,8 +3245,9 @@ A sftp file handle, NULL on error with ssh and sftp error set. # See also [`sftp_get_error`](@ref)() """ -function sftp_open(session, file, accesstype, mode) - @ccall libssh.sftp_open(session::sftp_session, file::Ptr{Cchar}, accesstype::Cint, mode::mode_t)::sftp_file +function sftp_open(session::sftp_session, file::Ptr{Cchar}, accesstype::Cint, mode::mode_t) + cfunc = @cfunction(_threadcall_sftp_open, sftp_file, (sftp_session, Ptr{Cchar}, Cint, mode_t)) + return @threadcall(cfunc, sftp_file, (sftp_session, Ptr{Cchar}, Cint, mode_t), session, file, accesstype, mode) end """ @@ -3300,8 +3414,19 @@ function sftp_aio_begin_read(file, len, aio) @ccall libssh.sftp_aio_begin_read(file::sftp_file, len::Csize_t, aio::Ptr{sftp_aio})::Cssize_t end +function _threadcall_sftp_aio_wait_read(aio::Ptr{sftp_aio}, buf::Ptr{Cvoid}, buf_size::Csize_t) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_aio_wait_read(aio::Ptr{sftp_aio}, buf::Ptr{Cvoid}, buf_size::Csize_t)::Cssize_t) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_aio_wait_read(aio, buf, buf_size) + sftp_aio_wait_read(aio::Ptr{sftp_aio}, buf::Ptr{Cvoid}, buf_size::Csize_t) + +Auto-generated wrapper around `sftp_aio_wait_read()`. Original upstream documentation is below. + +--- Wait for an asynchronous read to complete and store the read data in the supplied buffer. @@ -3322,8 +3447,9 @@ Number of bytes read, 0 on EOF, [`SSH_ERROR`](@ref) if an error occurred, [`SSH_ # See also [`sftp_aio_begin_read`](@ref)(), [`sftp_aio_free`](@ref)() """ -function sftp_aio_wait_read(aio, buf, buf_size) - @ccall libssh.sftp_aio_wait_read(aio::Ptr{sftp_aio}, buf::Ptr{Cvoid}, buf_size::Csize_t)::Cssize_t +function sftp_aio_wait_read(aio::Ptr{sftp_aio}, buf::Ptr{Cvoid}, buf_size::Csize_t) + cfunc = @cfunction(_threadcall_sftp_aio_wait_read, Cssize_t, (Ptr{sftp_aio}, Ptr{Cvoid}, Csize_t)) + return @threadcall(cfunc, Cssize_t, (Ptr{sftp_aio}, Ptr{Cvoid}, Csize_t), aio, buf, buf_size) end """ @@ -3818,8 +3944,19 @@ function sftp_expand_path(sftp, path) @ccall libssh.sftp_expand_path(sftp::sftp_session, path::Ptr{Cchar})::Ptr{Cchar} end +function _threadcall_sftp_home_directory(sftp::sftp_session, username::Ptr{Cchar}) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_home_directory(sftp::sftp_session, username::Ptr{Cchar})::Ptr{Cchar}) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_home_directory(sftp, username) + sftp_home_directory(sftp::sftp_session, username::Ptr{Cchar}) + +Auto-generated wrapper around `sftp_home_directory()`. Original upstream documentation is below. + +--- Get the specified user's home directory @@ -3835,8 +3972,9 @@ This calls the "home-directory" extension. You should check if the extension is # Returns On success, a newly allocated string containing the absolute real-path of the home directory of the user. NULL on error. The caller needs to free the memory using [`ssh_string_free_char`](@ref)(). """ -function sftp_home_directory(sftp, username) - @ccall libssh.sftp_home_directory(sftp::sftp_session, username::Ptr{Cchar})::Ptr{Cchar} +function sftp_home_directory(sftp::sftp_session, username::Ptr{Cchar}) + cfunc = @cfunction(_threadcall_sftp_home_directory, Ptr{Cchar}, (sftp_session, Ptr{Cchar})) + return @threadcall(cfunc, Ptr{Cchar}, (sftp_session, Ptr{Cchar}), sftp, username) end """ @@ -4264,7 +4402,7 @@ end """ ssh_message_reply_default(msg; throw = true) -Auto-generated wrapper around `ssh_message_reply_default`. Original upstream documentation is below. +Auto-generated wrapper around `ssh_message_reply_default()`. Original upstream documentation is below. --- @@ -4290,7 +4428,7 @@ end """ ssh_message_auth_user(msg; throw = true) -Auto-generated wrapper around `ssh_message_auth_user`. Original upstream documentation is below. +Auto-generated wrapper around `ssh_message_auth_user()`. Original upstream documentation is below. --- @@ -4318,7 +4456,7 @@ end """ ssh_message_auth_password(msg; throw = true) -Auto-generated wrapper around `ssh_message_auth_password`. Original upstream documentation is below. +Auto-generated wrapper around `ssh_message_auth_password()`. Original upstream documentation is below. --- @@ -4370,11 +4508,11 @@ function ssh_message_auth_pubkey(msg) end """ - ssh_message_auth_kbdint_is_response(msg; throw = true) + ssh_message_auth_kbdint_is_response(msg) -Auto-generated wrapper around [`ssh_message_auth_kbdint_is_response`](https://api.libssh.org/stable/group__libssh__server.html#ga5132c82c49de985e9e10f51f393e52a4). +Auto-generated wrapper around [`ssh_message_auth_kbdint_is_response()`](https://api.libssh.org/stable/group__libssh__server.html#ga5132c82c49de985e9e10f51f393e52a4). """ -function ssh_message_auth_kbdint_is_response(msg; throw = true) +function ssh_message_auth_kbdint_is_response(msg) ret = @ccall(libssh.ssh_message_auth_kbdint_is_response(msg::ssh_message)::Cint) return Bool(ret) end @@ -4396,7 +4534,7 @@ end """ ssh_message_auth_reply_success(msg, partial; throw = true) -Auto-generated wrapper around `ssh_message_auth_reply_success`. +Auto-generated wrapper around `ssh_message_auth_reply_success()`. """ function ssh_message_auth_reply_success(msg, partial; throw = true) ret = @ccall(libssh.ssh_message_auth_reply_success(msg::ssh_message, partial::Cint)::Cint) @@ -4427,7 +4565,7 @@ end """ ssh_message_auth_set_methods(msg, methods; throw = true) -Auto-generated wrapper around [`ssh_message_auth_set_methods`](https://api.libssh.org/stable/group__libssh__server.html#gab993157d98e5b4b3399d216c9243effc). +Auto-generated wrapper around [`ssh_message_auth_set_methods()`](https://api.libssh.org/stable/group__libssh__server.html#gab993157d98e5b4b3399d216c9243effc). """ function ssh_message_auth_set_methods(msg, methods; throw = true) ret = @ccall(libssh.ssh_message_auth_set_methods(msg::ssh_message, methods::Cint)::Cint) From 41eeb5db7f37899cac9aa39e4918aef50742f745 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:29:29 +0200 Subject: [PATCH 04/11] Make Session kinda threadsafe with locks --- src/session.jl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/session.jl b/src/session.jl index 61cddb5..3a121ce 100644 --- a/src/session.jl +++ b/src/session.jl @@ -24,6 +24,7 @@ mutable struct Session known_hosts::Union{String, Nothing} gssapi_server_identity::Union{String, Nothing} + _lock::ReentrantLock _auth_methods::Union{Vector{AuthMethod}, Nothing} _attempted_auth_methods::Vector{AuthMethod} _require_init_kbdint::Bool @@ -79,6 +80,9 @@ function Base.show(io::IO, session::Session) end end +Base.lock(session::Session) = lock(session._lock) +Base.unlock(session::Session) = unlock(session._lock) + # Non-throwing finalizer for Session objects function _finalizer(session::Session) try @@ -181,7 +185,7 @@ function Base.close(session::Session) throw(ArgumentError("Calling close() on a non-owning Session is not allowed to avoid accidental double-frees, see the docs for more information.")) end - if isopen(session) + @lock session if isopen(session) disconnect(session) lib.ssh_free(session) session.ptr = nothing @@ -228,7 +232,7 @@ const SAVED_PROPERTIES = (:log_verbosity, :gssapi_server_identity, :ssh_dir, :kn function Base.propertynames(::Session, private::Bool=false) private_fields = (:ptr, :channels, :server_callbacks, - :_auth_methods, :_attempted_auth_methods, + :_lock, :_auth_methods, :_attempted_auth_methods, :_kbdint_prompts, :_require_init_kbdint) libssh_options = tuple(keys(SESSION_PROPERTY_OPTIONS)...) public_fields = (:owning,) @@ -337,17 +341,13 @@ function Base.wait(session::Session; poll_timeout=0.1) throw(ArgumentError("poll_timeout=$(poll_timeout), it must be greater than 0")) end - if lib.ssh_is_blocking(session) == 1 - return - end - poll_flags = lib.ssh_get_poll_flags(session) readable = (poll_flags & lib.SSH_READ_PENDING) > 0 writable = (poll_flags & lib.SSH_WRITE_PENDING) > 0 fd = RawFD(lib.ssh_get_fd(session)) while isopen(session) - result = _safe_poll_fd(fd, poll_timeout; readable, writable) + result = @lock session _safe_poll_fd(fd, poll_timeout; readable, writable) if isnothing(result) # This means the session's file descriptor has been closed (see the # comments for _safe_poll_fd()). @@ -1019,7 +1019,7 @@ function _session_trywait(f::Function, session::Session) ret = SSH_ERROR while true - ret = f() + ret = @lock session f() if ret != SSH_AGAIN && ret != lib.SSH_AUTH_AGAIN break From 20ebe3d07e18293a489f73d0003b5dfaed9702da Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:31:13 +0200 Subject: [PATCH 05/11] Rename Session.channels to Session.closeables For SFTP support we need to support addings things other than SshChannels to Session's. --- src/channel.jl | 6 +++--- src/server.jl | 8 +++++--- src/session.jl | 22 ++++++++++++---------- test/LibSSHTests.jl | 2 +- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/channel.jl b/src/channel.jl index 6ef0aa4..56a5d3e 100644 --- a/src/channel.jl +++ b/src/channel.jl @@ -40,7 +40,7 @@ mutable struct SshChannel self = new(ptr, own, session, ReentrantLock(), false, nothing) if own - push!(session.channels, self) + push!(session.closeables, self) finalizer(_finalizer, self) end @@ -163,9 +163,9 @@ function Base.close(sshchan::SshChannel) # Remove from the sessions list of active channels. findfirst() # should only return nothing if the function is being called # recursively (i.e. through a callback) and it was already removed. - idx = findfirst(x -> x === sshchan, sshchan.session.channels) + idx = findfirst(x -> x === sshchan, sshchan.session.closeables) if !isnothing(idx) - popat!(sshchan.session.channels, idx) + popat!(sshchan.session.closeables, idx) end if !isnothing(sshchan.session) && !isconnected(sshchan.session) diff --git a/src/server.jl b/src/server.jl index 9e30440..633ad1c 100644 --- a/src/server.jl +++ b/src/server.jl @@ -69,9 +69,11 @@ function event_dopoll(event::SessionEvent) # during the poll, trying to unlock an unlocked channel will cause an # error. locked_channels = SshChannel[] - for sshchan in event.session.channels - lock(sshchan.close_lock) - push!(locked_channels, sshchan) + for obj in event.session.closeables + if obj isa SshChannel + lock(obj.close_lock) + push!(locked_channels, obj) + end end # Always check if the event is still assigned within the loop since it diff --git a/src/session.jl b/src/session.jl index 3a121ce..457007c 100644 --- a/src/session.jl +++ b/src/session.jl @@ -16,7 +16,7 @@ be owning or non-owning of its internal pointer to a `lib.ssh_session`. mutable struct Session ptr::Union{lib.ssh_session, Nothing} owning::Bool - channels::Vector{Any} + closeables::Vector{Any} server_callbacks::Union{Callbacks.ServerCallbacks, Nothing} log_verbosity::Int @@ -49,7 +49,9 @@ mutable struct Session # Set to non-blocking mode lib.ssh_set_blocking(ptr, 0) - session = new(ptr, own, [], nothing, -1, nothing, nothing, nothing, nothing, AuthMethod[], true) + session = new(ptr, own, [], nothing, + -1, nothing, nothing, nothing, + ReentrantLock(), nothing, AuthMethod[], true) if !isnothing(log_verbosity) session.log_verbosity = log_verbosity end @@ -231,7 +233,7 @@ const SESSION_PROPERTY_OPTIONS = Dict(:host => (SSH_OPTIONS_HOST, Cstring), const SAVED_PROPERTIES = (:log_verbosity, :gssapi_server_identity, :ssh_dir, :known_hosts) function Base.propertynames(::Session, private::Bool=false) - private_fields = (:ptr, :channels, :server_callbacks, + private_fields = (:ptr, :closeables, :server_callbacks, :_lock, :_auth_methods, :_attempted_auth_methods, :_kbdint_prompts, :_require_init_kbdint) libssh_options = tuple(keys(SESSION_PROPERTY_OPTIONS)...) @@ -398,15 +400,15 @@ Wrapper around [`lib.ssh_disconnect()`](@ref). """ function disconnect(session::Session) if isconnected(session) - # We close all the channels in reverse order because close(::SshChannel) - # deletes each channel from the vector and we don't want to invalidate - # any indices while deleting. The channels need to be closed here - # because lib.ssh_disconnect() will free all of them. - for i in reverse(eachindex(session.channels)) - # Note that only owning channels are added to session.channels, which + # We close all the closeables in reverse order because closing them will + # delete each object from the vector and we don't want to invalidate any + # indices while deleting. The channels in particular need to be closed + # here because lib.ssh_disconnect() will free all of them. + for i in reverse(eachindex(session.closeables)) + # Note that only owning channels are added to session.closeables, which # means that this should never throw because the channel is non-owning # (of course it may still throw for other reasons). - close(session.channels[i]) + close(session.closeables[i]) end lib.ssh_disconnect(session) diff --git a/test/LibSSHTests.jl b/test/LibSSHTests.jl index a3d8224..2fae533 100644 --- a/test/LibSSHTests.jl +++ b/test/LibSSHTests.jl @@ -454,7 +454,7 @@ end close(sshchan) @test isnothing(sshchan.ptr) - @test isempty(session.channels) + @test isempty(session.closeables) end end From 3b38fd905c9448e6abdc3943fc23d6eee04de499 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:32:22 +0200 Subject: [PATCH 06/11] Wrap the DemoServer client handler task in an errormonitor In case it fails when disconnecting/closing. --- src/server.jl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server.jl b/src/server.jl index 633ad1c..6b61371 100644 --- a/src/server.jl +++ b/src/server.jl @@ -430,7 +430,7 @@ function listen(handler::Function, bind::Bind; poll_timeout=0.1) end # Pass off to the handler - Threads.@spawn :interactive try + t = Threads.@spawn :interactive try handler(session) catch ex @error "Error handling SSH session!" exception=(ex, catch_backtrace()) @@ -438,6 +438,7 @@ function listen(handler::Function, bind::Bind; poll_timeout=0.1) disconnect(session) close(session) end + errormonitor(t) end end From 7f9b913c44f547d8602ab23f21396502a1043631 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:32:53 +0200 Subject: [PATCH 07/11] Add support for synchronously printing logs in the DemoServer Sometimes this is useful for debugging when you want to ensure that task switches aren't occurring. --- src/server.jl | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/server.jl b/src/server.jl index 6b61371..9c214f1 100644 --- a/src/server.jl +++ b/src/server.jl @@ -952,7 +952,7 @@ function stop(demo_server::DemoServer) end end -function _add_log_event!(client::Client, callback_name::Symbol, event) +function _add_log_event!(client::Client, callback_name::Symbol, event; printf=false) @lock client.log_lock begin if !haskey(client.callback_log, callback_name) client.callback_log[callback_name] = [] @@ -964,8 +964,15 @@ function _add_log_event!(client::Client, callback_name::Symbol, event) if client.verbose timestamp = Dates.format(Dates.now(), Dates.ISOTimeFormat) - @info "$timestamp DemoServer client $(client.id): $callback_name $event" - flush(stdout) + msg = "$timestamp DemoServer client $(client.id): $callback_name $event" + + if printf + @ccall printf("[ Info: $(msg)\n"::Cstring)::Cint + Libc.flush_cstdio() + else + @info msg + flush(stdout) + end end end end From 89f0392c09dbb1e5eeb397025b6d7375bf290461 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:34:52 +0200 Subject: [PATCH 08/11] Allow letting close() and closewrite() for SshChannel to fail In practice these can fail for good reason if the remote end has already disconnected the socket. --- docs/src/changelog.md | 3 +++ src/channel.jl | 36 +++++++++++++++++++++++++++++++----- src/server.jl | 2 +- 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/docs/src/changelog.md b/docs/src/changelog.md index 904012d..bb0385c 100644 --- a/docs/src/changelog.md +++ b/docs/src/changelog.md @@ -16,6 +16,9 @@ Changelog](https://keepachangelog.com). [`Base.run(::Cmd)`](@ref) ([#12]). - Made it possible to assign callbacks to [`Callbacks.ServerCallbacks`](@ref) and [`Callbacks.ChannelCallbacks`](@ref) by property ([#14]). +- [`close(::SshChannel)`](@ref) and [`closewrite(::SshChannel)`](@ref) now + support an `allow_fail` argument that will print a warning instead of throw an + exception if modifying the `lib.ssh_channel` fails ([#16]). ### Fixed diff --git a/src/channel.jl b/src/channel.jl index 56a5d3e..b39c68e 100644 --- a/src/channel.jl +++ b/src/channel.jl @@ -142,8 +142,15 @@ $(TYPEDSIGNATURES) Closes the channel, and then frees its memory. To avoid the risk of double-frees, this function may only be called on *owning* `SshChannel`s. It will hold the `close_lock` of the channel during execution. + +# Arguments +- `sshchan`: The [`SshChannel`](@ref) to close. +- `allow_fail=false`: Whether to throw an exception if the call to + [`lib.ssh_channel_close()`](@ref) fails. In some cases it can fail for valid + reasons, such as the socket already having been closed by the other end (this + will result in a `Socket error: disconnected` error). """ -function Base.close(sshchan::SshChannel) +function Base.close(sshchan::SshChannel; allow_fail=false) # Developer note: this function is called by the SshChannel finalizer, which # means we aren't allowed to do task switches. @@ -176,13 +183,19 @@ function Base.close(sshchan::SshChannel) sshchan.ptr = nothing elseif isopen(sshchan) # This will trigger callbacks - closewrite(sshchan) + closewrite(sshchan; allow_fail) if isopen(sshchan) # This will trigger callbacks ret = lib.ssh_channel_close(sshchan) if ret != SSH_OK - throw(LibSSHException("Closing SshChannel failed: $(ret)")) + msg = "Closing SshChannel failed with $(ret): '$(get_error(sshchan.session))'" + if allow_fail + # Note that we spawn to avoid task switches + Threads.@spawn @warn msg + else + throw(LibSSHException(msg)) + end end end end @@ -291,8 +304,15 @@ Sends an EOF message. Calling this function will trigger any waiting callbacks. - `ArgumentError`: if the channel is not writable. Wrapper around [`lib.ssh_channel_send_eof()`](@ref). + +# Arguments +- `sshchan`: The [`SshChannel`](@ref) to send an EOF on. +- `allow_fail=false`: Whether to throw an exception if the call to + [`lib.ssh_channel_send_eof()`](@ref) fails. In some cases it can fail for + valid reasons, such as the socket already having been closed by the other end + (this will result in a `Socket error: disconnected` error). """ -function Base.closewrite(sshchan::SshChannel) +function Base.closewrite(sshchan::SshChannel; allow_fail=false) # If we've already sent an EOF, do nothing if sshchan.local_eof return @@ -304,8 +324,14 @@ function Base.closewrite(sshchan::SshChannel) ret = lib.ssh_channel_send_eof(sshchan) if ret != SSH_OK - throw(LibSSHException("Error when sending EOF on channel: $(ret)")) + error_msg = get_error(sshchan.session) + if allow_fail + Threads.@spawn @warn "closewrite() on SshChannel failed: '$(error_msg)'" + else + throw(LibSSHException("Error when sending EOF on channel: '$(error_msg)'")) + end end + sshchan.local_eof = true end diff --git a/src/server.jl b/src/server.jl index 9c214f1..b6d74ba 100644 --- a/src/server.jl +++ b/src/server.jl @@ -1191,7 +1191,7 @@ function Base.close(op::SftpOperation) op.sftp_session = nothing end - close(op.sshchan) + close(op.sshchan; allow_fail=true) end end From fe42ee30cb1348262a2c483492aa8b80d0df70ab Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sat, 5 Oct 2024 23:36:30 +0200 Subject: [PATCH 09/11] Make the SFTP DemoServer logs slightly more informative --- src/server.jl | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/server.jl b/src/server.jl index b6d74ba..444fd69 100644 --- a/src/server.jl +++ b/src/server.jl @@ -1170,12 +1170,14 @@ function on_channel_subsystem_request(session, sshchan, subsystem, client)::Bool end function on_sftp_data(session, sshchan, data, is_stderr, client) - _add_log_event!(client, :channel_sftp_data, "$(length(data)) bytes") - lib.sftp_channel_default_data_callback(session, sshchan, - pointer(data), length(data), - is_stderr, - Ref(Ptr{Cvoid}(client.sftp_session))) + ret = lib.sftp_channel_default_data_callback(session, sshchan, + pointer(data), length(data), + is_stderr, + Ref(Ptr{Cvoid}(client.sftp_session))) + _add_log_event!(client, :channel_sftp_data, "$(length(data)) bytes received, $(ret) bytes processed") + + return ret end mutable struct SftpOperation From 4e55964662fd4c4e686a55281faba706be3a8522 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sun, 6 Oct 2024 20:41:09 +0200 Subject: [PATCH 10/11] Add initial SFTP client support --- docs/make.jl | 1 + docs/src/changelog.md | 1 + docs/src/contributing.md | 3 +- docs/src/examples.jl | 50 ++- docs/src/index.md | 2 +- docs/src/sftp.md | 67 ++++ gen/gen.jl | 3 +- gen/generator.toml | 4 +- src/LibSSH.jl | 3 + src/bindings.jl | 18 +- src/session.jl | 16 + src/sftp.jl | 742 +++++++++++++++++++++++++++++++++++++++ test/LibSSHTests.jl | 244 +++++++++++++ 13 files changed, 1144 insertions(+), 10 deletions(-) create mode 100644 docs/src/sftp.md create mode 100644 src/sftp.jl diff --git a/docs/make.jl b/docs/make.jl index 56a4dcc..0dfca93 100644 --- a/docs/make.jl +++ b/docs/make.jl @@ -84,6 +84,7 @@ makedocs(; "index.md", "Examples" => "examples.md", "sessions_and_channels.md", + "sftp.md", "server_support.md", "utilities.md", "bindings.md", diff --git a/docs/src/changelog.md b/docs/src/changelog.md index bb0385c..5fe9b4f 100644 --- a/docs/src/changelog.md +++ b/docs/src/changelog.md @@ -19,6 +19,7 @@ Changelog](https://keepachangelog.com). - [`close(::SshChannel)`](@ref) and [`closewrite(::SshChannel)`](@ref) now support an `allow_fail` argument that will print a warning instead of throw an exception if modifying the `lib.ssh_channel` fails ([#16]). +- Basic [SFTP](sftp.md) support. ### Fixed diff --git a/docs/src/contributing.md b/docs/src/contributing.md index 25ed83b..cdfd0b6 100644 --- a/docs/src/contributing.md +++ b/docs/src/contributing.md @@ -4,8 +4,7 @@ Libssh is a fairly large project and most of the API still doesn't have high-level wrappers in LibSSH.jl. For example: - Reverse port forwarding - Unix socket forwarding -- SFTP support -- SCP support +- Complete SFTP/SCP support If you'd like to contribute new wrappers, the usual workflow is: 1. Add support for the feature in the [Demo server](@ref) and test it with the diff --git a/docs/src/examples.jl b/docs/src/examples.jl index f83374c..a235ae6 100644 --- a/docs/src/examples.jl +++ b/docs/src/examples.jl @@ -2,7 +2,7 @@ #md # CurrentModule = LibSSH #md # ``` -# # A simple client +# ## Connecting and authenticating # # First we'll import the LibSSH package: @@ -70,15 +70,61 @@ ssh.userauth_list(session) @assert ssh.userauth_password(session, "foo") == ssh.AuthStatus_Success -# Now we're authenticated to the server and we can actually do something, like +# Going through all the authentication methods can be quite complicated, in +# practice it may be easier to use [`authenticate()`](@ref) which will handle +# all of that for you. + +# ## Running commands +# Now that we're authenticated to the server we can actually do something, like # running a command (see [Command execution](@ref)): @assert read(`echo 'Hello world!'`, session, String) == "Hello world!\n" +# ## SFTP +# LibSSH.jl allows reading and writing remote files with the same API as local +# files with `Base`. Lets start by making a temporary directory and creating a +# file in it 'remotely': + +tmpdir = mktempdir() +path = joinpath(tmpdir, "foo") + +sftp = ssh.SftpSession(session) +file = open(path, sftp; write=true) +write(file, "foo") # this returns the number of bytes written + +# We can read the file 'remotely': + +open(path, sftp) do readonly_file + read(readonly_file, String) +end + +# And do other IO-related things: + +seekstart(file) +position(file) +#- +isreadable(file) +#- +iswritable(file) + +# After using it we have to close it explicitly because the finalizer won't do +# it for us (see the [`Base.close(::SftpFile)`](@ref) docstring for details): + +close(file) + +# ## Disconnecting # Now we can disconnect our client session: +close(sftp) close(session) # And stop the server: demo.stop(demo_server) + +# Note that sometimes the `DemoServer` will display a warning that closing an +# `SshChannel` failed because of `Socket error: disconnected`. That can be +# safely ignored, it just means that the socket was closed on the client side +# before the server could close the `SshChannel`, but the `SshChannel` memory +# will still be freed. It typically happens when doing SFTP operations since the +# [`SftpSession`](@ref) manages its own `lib.ssh_channel`. diff --git a/docs/src/index.md b/docs/src/index.md index aa9607f..d8456b9 100644 --- a/docs/src/index.md +++ b/docs/src/index.md @@ -58,6 +58,6 @@ so <3). ## Contents ```@contents -Pages = ["examples.md", "sessions_and_channels.md", "server_support.md", "utilities.md", "bindings.md"] +Pages = ["examples.md", "sessions_and_channels.md", "sftp.md", "server_support.md", "utilities.md", "bindings.md"] Depth = 10 ``` diff --git a/docs/src/sftp.md b/docs/src/sftp.md new file mode 100644 index 0000000..41da05e --- /dev/null +++ b/docs/src/sftp.md @@ -0,0 +1,67 @@ +```@meta +CurrentModule = LibSSH +``` + +# SFTP + +A subset of the [SFTP +API](https://api.libssh.org/stable/group__libssh__sftp.html) is wrapped and +available in LibSSH.jl. See the [SFTP example](examples.md#SFTP) for an example +of basic usage. + +Unlike the rest of the API, the SFTP C functions are blocking and only work with +blocking [`Session`](@ref)'s. This means that the library has to lock the +session while calling them and no other operations (blocking or unblocking) can +occur while they're being called. In practice this restriction may not be too +onerous since most calls shouldn't take long anyway, and the read/write +implementations use SFTP's asynchronous IO API so they shouldn't block for +long. If it's critical that SFTP operations don't interfere with other +operations (e.g. port forwarding) a workaround would be to open a separate +[`Session`](@ref) for SFTP. + +Note that we call all blocking C functions using `@threadcall` so that they +don't block the scheduler, hence as a programmer you don't need to worry about +them hogging a whole thread until they complete. + +```@contents +Pages = ["sftp.md"] +Depth = 3 +``` + +## SftpSession +```@docs +SftpError +SftpSession +SftpSession(::Session) +SftpSession(::Function) +Base.close(::SftpSession) +Base.isopen(::SftpSession) +Base.lock(::SftpSession) +Base.unlock(::SftpSession) +Base.stat(::String, ::SftpSession) +get_extensions(::SftpSession) +get_limits(::SftpSession) +get_error(::SftpSession) +``` + +## SftpFile +```@docs +SftpFile +Base.open(::String, ::SftpSession) +Base.open(::Function, ::String, ::SftpSession) +Base.close(::SftpFile) +Base.read(::SftpFile) +Base.read(::SftpFile, ::Type{String}) +Base.read!(::SftpFile, ::Vector{UInt8}) +Base.write(::SftpFile, ::DenseVector) +Base.write(::SftpFile, ::AbstractString) + +Base.isopen(::SftpFile) +Base.isreadable(::SftpFile) +Base.isreadonly(::SftpFile) +Base.iswritable(::SftpFile) +Base.position(::SftpFile) +Base.seek(::SftpFile, ::Integer) +Base.seekstart(::SftpFile) +Base.seekend(::SftpFile) +``` diff --git a/gen/gen.jl b/gen/gen.jl index 4e7d33f..f04522e 100644 --- a/gen/gen.jl +++ b/gen/gen.jl @@ -25,7 +25,8 @@ ssh_ok_functions = [:ssh_message_auth_reply_success, :ssh_message_auth_set_metho # These functions require the ssh_session to be in blocking mode, so we always # call them with @threadcall. threadcall_functions = [:sftp_new, :sftp_init, :sftp_open, :sftp_close, - :sftp_home_directory, :sftp_stat, :sftp_aio_wait_read] + :sftp_home_directory, :sftp_stat, + :sftp_aio_wait_read, :sftp_aio_wait_write] all_rewritable_functions = vcat(string_functions, bool_functions, ssh_ok_functions, threadcall_functions) """ diff --git a/gen/generator.toml b/gen/generator.toml index fdb5e32..b207ba1 100644 --- a/gen/generator.toml +++ b/gen/generator.toml @@ -24,7 +24,9 @@ auto_mutability_includelist = ["ssh_server_callbacks_struct", [codegen] use_ccall_macro = true -field_access_method_list = ["ssh_server_callbacks_struct", "ssh_channel_callbacks_struct"] +field_access_method_list = ["ssh_server_callbacks_struct", + "ssh_channel_callbacks_struct", + "sftp_attributes_struct"] [codegen.macro] functionlike_macro_includelist = ["SSH_VERSION", "SSH_VERSION_INT", "SSH_VERSION_DOT", "ssh_callbacks_init"] diff --git a/src/LibSSH.jl b/src/LibSSH.jl index bde4fe3..77cd840 100644 --- a/src/LibSSH.jl +++ b/src/LibSSH.jl @@ -185,4 +185,7 @@ include("channel.jl") include("message.jl") include("server.jl") +import Base: Filesystem +include("sftp.jl") + end diff --git a/src/bindings.jl b/src/bindings.jl index 183e8f9..cbf59d9 100644 --- a/src/bindings.jl +++ b/src/bindings.jl @@ -3495,8 +3495,19 @@ function sftp_aio_begin_write(file, buf, len, aio) @ccall libssh.sftp_aio_begin_write(file::sftp_file, buf::Ptr{Cvoid}, len::Csize_t, aio::Ptr{sftp_aio})::Cssize_t end +function _threadcall_sftp_aio_wait_write(aio::Ptr{sftp_aio}) + gc_state = @ccall(jl_gc_safe_enter()::Int8) + ret = @ccall(libssh.sftp_aio_wait_write(aio::Ptr{sftp_aio})::Cssize_t) + @ccall jl_gc_safe_leave(gc_state::Int8)::Cvoid + return ret +end + """ - sftp_aio_wait_write(aio) + sftp_aio_wait_write(aio::Ptr{sftp_aio}) + +Auto-generated wrapper around `sftp_aio_wait_write()`. Original upstream documentation is below. + +--- Wait for an asynchronous write to complete. @@ -3515,8 +3526,9 @@ Number of bytes written on success, [`SSH_ERROR`](@ref) if an error occurred, [` # See also [`sftp_aio_begin_write`](@ref)(), [`sftp_aio_free`](@ref)() """ -function sftp_aio_wait_write(aio) - @ccall libssh.sftp_aio_wait_write(aio::Ptr{sftp_aio})::Cssize_t +function sftp_aio_wait_write(aio::Ptr{sftp_aio}) + cfunc = @cfunction(_threadcall_sftp_aio_wait_write, Cssize_t, (Ptr{sftp_aio},)) + return @threadcall(cfunc, Cssize_t, (Ptr{sftp_aio},), aio) end """ diff --git a/src/session.jl b/src/session.jl index 457007c..69e9972 100644 --- a/src/session.jl +++ b/src/session.jl @@ -329,6 +329,22 @@ function Base.setproperty!(session::Session, name::Symbol, value) return value end +# Helper macro to lock a session and temporarily set it to blocking mode while +# executing some expression. +macro lockandblock(session, expr) + quote + @lock $(esc(session)) begin + lib.ssh_set_blocking($(esc(session)), 1) + + try + $(esc(expr)) + finally + lib.ssh_set_blocking($(esc(session)), 0) + end + end + end +end + """ $(TYPEDSIGNATURES) diff --git a/src/sftp.jl b/src/sftp.jl new file mode 100644 index 0000000..419119a --- /dev/null +++ b/src/sftp.jl @@ -0,0 +1,742 @@ +""" +$(TYPEDEF) + +Enum for possible SFTP error codes. Note that despite its name, `SftpError_Ok` +does not indicate an error. + +- `SftpError_Ok` +- `SftpError_Eof` +- `SftpError_NoSuchFile` +- `SftpError_PermissionDenied` +- `SftpError_GenericFailure` +- `SftpError_BadMessage` +- `SftpError_NoConnection` +- `SftpError_ConnectionLost` +- `SftpError_OpUnsupported` +- `SftpError_InvalidHandle` +- `SftpError_NoSuchPath` +- `SftpError_FileAlreadyExists` +- `SftpError_WriteProtect` +- `SftpError_NoMedia` +""" +@enum SftpError begin + SftpError_Ok = Int(lib.SSH_FX_OK) + SftpError_Eof = Int(lib.SSH_FX_EOF) + SftpError_NoSuchFile = Int(lib.SSH_FX_NO_SUCH_FILE) + SftpError_PermissionDenied = Int(lib.SSH_FX_PERMISSION_DENIED) + SftpError_GenericFailure = Int(lib.SSH_FX_FAILURE) + SftpError_BadMessage = Int(lib.SSH_FX_BAD_MESSAGE) + SftpError_NoConnection = Int(lib.SSH_FX_NO_CONNECTION) + SftpError_ConnectionLost = Int(lib.SSH_FX_CONNECTION_LOST) + SftpError_OpUnsupported = Int(lib.SSH_FX_OP_UNSUPPORTED) + SftpError_InvalidHandle = Int(lib.SSH_FX_INVALID_HANDLE) + SftpError_NoSuchPath = Int(lib.SSH_FX_NO_SUCH_PATH) + SftpError_FileAlreadyExists = Int(lib.SSH_FX_FILE_ALREADY_EXISTS) + SftpError_WriteProtect = Int(lib.SSH_FX_WRITE_PROTECT) + SftpError_NoMedia = Int(lib.SSH_FX_NO_MEDIA) +end + + +## SftpSession + + +""" +$(TYPEDEF) + +This represents a SFTP session, through which one can do SFTP operations. It is +only usable while its parent [`Session`](@ref) is still open. +""" +mutable struct SftpSession + ptr::Union{lib.sftp_session, Nothing} + session::Session + files::Vector{Any} + + _lock::ReentrantLock + + @doc """ + $(TYPEDSIGNATURES) + + Create a `SftpSession` from an existing [`Session`](@ref). + + # Throws + - `ArgumentError`: If `session` isn't open. + - [`LibSSHException`](@ref): If creating the SFTP session fails. + """ + function SftpSession(session::Session) + if !isopen(session) + throw(ArgumentError("Session is closed, cannot create an SftpSession with it")) + end + + ptr = @lockandblock session lib.sftp_new(session.ptr) + if ptr == C_NULL + error_msg = get_error(session) + throw(LibSSHException("Couldn't create an SFTP session, call to lib.sftp_new() failed: '$(error_msg)'")) + end + + self = new(ptr, session, [], ReentrantLock()) + + ret = @lockandblock session lib.sftp_init(ptr) + if ret != SSH_OK + error_code = get_error(self) + close(self) + throw(LibSSHException("Couldn't initialize the SFTP session, call to lib.sftp_init() failed with $(ret): $(error_code)")) + end + + push!(session.closeables, self) + + finalizer(close, self) + end +end + +""" +$(TYPEDSIGNATURES) + +Do-constructor, the function `f` will be called like `f(sftp)` with the new +[`SftpSession`](@ref). +""" +function SftpSession(f::Function, args...; kwargs...) + sftp = SftpSession(args...; kwargs...) + try + f(sftp) + finally + close(sftp) + end +end + +""" +$(TYPEDSIGNATURES) + +Lock an [`SftpSession`](@ref). +""" +Base.lock(sftp::SftpSession) = lock(sftp._lock) + +""" +$(TYPEDSIGNATURES) + +Unlock an [`SftpSession`](@ref). +""" +Base.unlock(sftp::SftpSession) = unlock(sftp._lock) + +Base.isassigned(sftp::SftpSession) = !isnothing(sftp.ptr) + +""" +$(TYPEDSIGNATURES) + +Check if `sftp` is open. +""" +Base.isopen(sftp::SftpSession) = isassigned(sftp) + +function Base.unsafe_convert(::Type{lib.sftp_session}, sftp::SftpSession) + if !isassigned(sftp) + throw(ArgumentError("The SftpSession doesn't have a valid pointer, cannot convert it to a lib.sftp_session")) + end + + return sftp.ptr +end + +""" +$(TYPEDSIGNATURES) + +Close an `SftpSession`. This will also close any open files. +""" +function Base.close(sftp::SftpSession) + if isassigned(sftp) + for i in reverse(eachindex(sftp.files)) + close(sftp.files[i]) + end + + lib.sftp_free(sftp.ptr) + sftp.ptr = nothing + end +end + +""" +$(TYPEDSIGNATURES) + +Get the current error code for `sftp`. + +# Throws +- `ArgumentError`: If `sftp` is closed. +""" +function get_error(sftp::SftpSession) + if !isassigned(sftp) + throw(ArgumentError("The SftpSession doesn't have a valid pointer, cannot get its error message")) + end + + SftpError(lib.sftp_get_error(sftp)) +end + +""" +$(TYPEDSIGNATURES) + +Get a list of supported server extensions and their versions. + +# Throws +- `ArgumentError`: If `sftp` is closed. +""" +function get_extensions(sftp::SftpSession) + if !isassigned(sftp) + throw(ArgumentError("The SftpSession doesn't have a valid pointer, cannot get its extensions")) + end + + extensions = Dict{String, String}() + count = lib.sftp_extensions_get_count(sftp) + for i in 0:count - 1 + extensions[lib.sftp_extensions_get_name(sftp, i)] = lib.sftp_extensions_get_data(sftp, i) + end + + return extensions +end + +""" +$(TYPEDSIGNATURES) + +Get the server limits. The returned object has the following fields: +- `max_packet_length` +- `max_read_length` +- `max_write_length` +- `max_open_handles` + +# Throws +- `ArgumentError`: If `sftp` is closed. +- [`LibSSHException`](@ref): If getting the limits failed. +""" +function get_limits(sftp::SftpSession) + if !isassigned(sftp) + throw(ArgumentError("The SftpSession doesn't have a valid pointer, cannot get its limits")) + end + + ptr = lib.sftp_limits(sftp) + if ptr == C_NULL + throw(LibSSHException("Couldn't get limits for $sftp")) + end + + limits = unsafe_load(ptr) + lib.sftp_limits_free(ptr) + + return limits +end + +function Base.homedir(sftp::SftpSession, username=nothing) + if !isassigned(sftp) + throw(ArgumentError("The SftpSession doesn't have a valid pointer, cannot get the home directory")) + end + + if "home-directory" ∉ keys(get_extensions(sftp)) + error("The SSH server doesn't support the 'home-directory' extension, cannot get the home directory") + end + + ret = C_NULL + GC.@preserve username begin + arg = isnothing(username) ? C_NULL : Base.unsafe_convert(Ptr{Cchar}, username) + ret = @lockandblock sftp.session lib.sftp_home_directory(sftp.ptr, arg) + end + + if ret == C_NULL + error_code = get_error(sftp) + throw(LibSSHException("Couldn't get the home directory: $(error_code)")) + end + + path = unsafe_string(Ptr{UInt8}(ret)) + lib.ssh_string_free_char(ret) + + return path +end + +""" +$(TYPEDSIGNATURES) + +Get information about the file object at `path`. This will return an object with +the following properties: +- `name::String` +- `longname::String` +- `flags::UInt32` +- `type::UInt8` +- `size::UInt64` +- `uid::UInt32` +- `gid::UInt32` +- `owner::String` +- `group::String` +- `permissions::UInt32` +- `atime64::UInt64` +- `atime::UInt32` +- `atime_nseconds::UInt32` +- `createtime::UInt64` +- `createtime_nseconds::UInt32` +- `mtime64::UInt64` +- `mtime::UInt32` +- `mtime_nseconds::UInt32` +- `acl::String` +- `extended_count::UInt32` +- `extended_type::String` +- `extended_data::String` + +Note: the [`Demo.DemoServer`](@ref) does not support setting all of these +properties. + +# Throws +- `ArgumentError`: If `sftp` is closed. +- [`LibSSHException`](@ref): If retrieving the file object information failed + (e.g. if the path doesn't exist). +""" +function Base.stat(path::String, sftp::SftpSession) + if !isassigned(sftp) + throw(ArgumentError("$sftp is closed, cannot stat() with it")) + end + + ptr = GC.@preserve path begin + cstr = Base.unsafe_convert(Ptr{Cchar}, path) + @lockandblock sftp.session lib.sftp_stat(sftp.ptr, cstr) + end + + if ptr == C_NULL + error_code = get_error(sftp) + throw(LibSSHException("Couldn't stat '$path': $error_code")) + end + + SftpAttributes(ptr) +end + + +## SftpAttributes + + +mutable struct SftpAttributes + ptr::Union{lib.sftp_attributes, Nothing} + + function SftpAttributes(ptr::lib.sftp_attributes) + self = new(ptr) + finalizer(close, self) + end +end + +Base.isassigned(attrs::SftpAttributes) = !isnothing(attrs.ptr) + +function Base.close(attrs::SftpAttributes) + if isassigned(attrs) + lib.sftp_attributes_free(attrs.ptr) + attrs.ptr = nothing + end +end + +function _show_attrs(io::IO, attrs::SftpAttributes) + mode = string(attrs.permissions, base=8, pad=6) + print(io, SftpAttributes, "(size=$(attrs.size) bytes, owner=$(attrs.owner), uid=$(attrs.uid), gid=$(attrs.gid), permissions=0o$(mode))") +end + +Base.show(io::IO, attrs::SftpAttributes) = _show_attrs(io, attrs) + +function _load_attr(x::Ptr{Ptr{Cchar}}) + x = unsafe_load(x) + x == C_NULL ? "" : unsafe_string(Ptr{UInt8}(x)) +end + +function _load_attr(x::Ptr{lib.ssh_string}) + x = unsafe_load(x) + x == C_NULL ? "" : unsafe_string(Ptr{UInt8}(lib.ssh_string_get_char(x))) +end + +_load_attr(x) = unsafe_load(x) + +function Base.getproperty(attrs::SftpAttributes, name::Symbol) + if name in fieldnames(lib.sftp_attributes_struct) + ptr = getfield(attrs, :ptr) + _load_attr(getproperty(ptr, name)) + else + getfield(attrs, name) + end +end + + +## SftpFile + + +const FlagsType = @NamedTuple{read::Bool, write::Bool, create::Bool, + truncate::Bool, append::Bool, exclusive::Bool} + +""" +$(TYPEDEF) +$(TYPEDFIELDS) + +Represents a remote file. This object _must_ be explicitly closed with +`close()` or it will leak memory. Don't create one of these yourself, use +[`Base.open(::String, ::SftpSession)`](@ref). +""" +mutable struct SftpFile + ptr::Union{lib.sftp_file, Nothing} + sftp::SftpSession + path::String + fullpath::String + flags::FlagsType + + function SftpFile(ptr::lib.sftp_file, sftp::SftpSession, path::String, flags) + session = sftp.session + lib.sftp_file_set_nonblocking(ptr) + self = new(ptr, sftp, path, "$(session.user)@$(session.host):$(path)", flags) + push!(sftp.files, self) + + finalizer(_finalize, self) + end +end + +# We can't close the file in the finalizer because that requires locking the +# session, which could lead to a task switch. +function _finalize(file::SftpFile) + if isassigned(file) + Threads.@spawn @error "$file has not been close()'d, this is a memory leak! The finalizer cannot close the file because it can require task switching." + end +end + +function Base.unsafe_convert(::Type{lib.sftp_file}, file::SftpFile) + if !isopen(file) + throw(ArgumentError("$file has been closed, cannot convert it to a pointer")) + end + + file.ptr +end + +Base.isassigned(file::SftpFile) = !isnothing(file.ptr) + +""" +$(TYPEDSIGNATURES) + +Check if `file` is open. +""" +Base.isopen(file::SftpFile) = isassigned(file) + +""" +$(TYPEDSIGNATURES) + +Check if `file` is open and readable. +""" +Base.isreadable(file::SftpFile) = isopen(file) && file.flags.read + +""" +$(TYPEDSIGNATURES) + +Check if `file` is open and readonly. +""" +Base.isreadonly(file::SftpFile) = isopen(file) && file.flags.read && !file.flags.write + +""" +$(TYPEDSIGNATURES) + +Check if `file` is open and writable. +""" +Base.iswritable(file::SftpFile) = isopen(file) && file.flags.write + +""" +$(TYPEDSIGNATURES) + +Close `file`. This _must_ be called explicitly, and not in a finalizer because +it may cause a task switch. +""" +function Base.close(file::SftpFile) + if isassigned(file) + @lock file.sftp begin + idx = findfirst(x -> x === file, file.sftp.files) + if !isnothing(idx) + popat!(file.sftp.files, idx) + else + @error "Couldn't find $file in the parent SFTP session, this may be a memory leak." + end + end + + @lockandblock file.sftp.session lib.sftp_close(file.ptr) + file.ptr = nothing + end +end + +function Base.show(io::IO, file::SftpFile) + state = isopen(file) ? "open" : "closed" + print(io, SftpFile, "($(file.fullpath) [$(state)])") +end + +""" +$(TYPEDSIGNATURES) + +Open a remote file. Most of the keyword arguments behave in exactly the same way +as their counterparts in `Base.open(::String)`, except for `exclusive` and +`mode`, which are unique to this method: +- `exclusive`: Open a file with the `O_EXCL` flag. +- `mode`: If `create=true` and the file doesn't exist, it will be created with + permissions `(mode & ~umask)`. + +# Throws +- `ArgumentError`: If `sftp` is closed. +- [`LibSSHException`](@ref): If opening the file fails. +""" +function Base.open(path::String, sftp::SftpSession; + read::Union{Bool, Nothing}=nothing, + write::Union{Bool, Nothing}=nothing, + create::Union{Bool, Nothing}=nothing, + truncate::Union{Bool, Nothing}=nothing, + append::Union{Bool, Nothing}=nothing, + exclusive::Bool=false, + mode::Unsigned=0o644) + if !isassigned(sftp) + throw(ArgumentError("$(sftp) has been closed, cannot use it to open a file")) + end + + flags = Base.open_flags(; read, write, create, truncate, append) + accesstype = if flags.read && !flags.write + Filesystem.JL_O_RDONLY + elseif !flags.read && flags.write + Filesystem.JL_O_WRONLY + elseif flags.read && flags.write + Filesystem.JL_O_RDWR + end + + if flags.create + accesstype |= Filesystem.JL_O_CREAT + end + if flags.truncate + accesstype |= Filesystem.JL_O_TRUNC + end + if exclusive + accesstype |= Filesystem.JL_O_EXCL + end + + ret = C_NULL + GC.@preserve path begin + cstr = Base.unsafe_convert(Ptr{Cchar}, path) + ret = @lockandblock sftp.session lib.sftp_open(sftp.ptr, cstr, Cint(accesstype), lib.mode_t(mode)) + end + + if ret == C_NULL + error_code = get_error(sftp) + throw(LibSSHException("Couldn't open file '$path' on host $(sftp.session.host): $error_code")) + end + + file = SftpFile(ret, sftp, path, (; flags..., exclusive)) + + if flags.append + seekend(file) + end + + return file +end + +""" +$(TYPEDSIGNATURES) + +Do-constructor for [`SftpFile`](@ref)'s, the function `f` will be called like +`f(file)`. +""" +function Base.open(f::Function, path::String, sftp::SftpSession; kwargs...) + file = open(path, sftp; kwargs...) + + try + f(file) + finally + close(file) + end +end + +""" +$(TYPEDSIGNATURES) + +Get the current position in `file.` + +# Throws +- `ArgumentError`: If `file` is closed. +""" +function Base.position(file::SftpFile) + if !isopen(file) + throw(ArgumentError("$file is not open, cannot get its position")) + end + + lib.sftp_tell64(file) +end + +""" +$(TYPEDSIGNATURES) + +Go to position `pos` in `file`. Note that this will not validate `pos`. + +# Throws +- `ArgumentError`: If `file` is closed. +""" +function Base.seek(file::SftpFile, pos::Integer) + if !isopen(file) + throw(ArgumentError("$file is not open, cannot seek() it")) + end + + lib.sftp_seek64(file, pos) + return nothing +end + +""" +$(TYPEDSIGNATURES) + +Go to the beginning of `file`. +""" +Base.seekstart(file::SftpFile) = seek(file, 0) + +""" +$(TYPEDSIGNATURES) + +Go to the end of `file`. +""" +Base.seekend(file::SftpFile) = seek(file, stat(file.path, file.sftp).size) + +""" +$(TYPEDSIGNATURES) + +Read at most `nb` bytes from the remote [`SftpFile`](@ref). Uses +[`Base.read!(::SftpFile, ::Vector{UInt8})`](@ref) internally. + +# Throws +- `ArgumentError`: If `file` is closed. +- [`LibSSHException`](@ref): If reading failed. +""" +function Base.read(file::SftpFile, nb::Integer=typemax(Int)) + if !isopen(file) + throw(ArgumentError("$file is closed, cannot read from it")) + end + + if nb == typemax(Int) + nb = stat(file.path, file.sftp).size - position(file) + end + + out = Vector{UInt8}(undef, (nb,)) + read!(file, out) + + return out +end + +""" +$(TYPEDSIGNATURES) + +Read `length(out)` bytes from the remote [`SftpFile`](@ref) into `out`. This +uses libssh's asynchronous IO functions under the hood so it may launch multiple +parallel requests. + +# Throws +- `ArgumentError`: If `file` is closed. +- [`LibSSHException`](@ref): If reading failed. +""" +function Base.read!(file::SftpFile, out::Vector{UInt8}) + if !isopen(file) + throw(ArgumentError("$file is closed, cannot read from it")) + end + + nb = length(out) + handles = [] + free_handles = () -> map(x -> lib.sftp_aio_free(x[3][]), handles) + + # Launch requests + bytes_requested = 0 + try + while bytes_requested < nb + handle = Ref{lib.sftp_aio}() + ret = lib.sftp_aio_begin_read(file, nb - bytes_requested, handle) + if ret == SSH_ERROR + error_code = get_error(file.sftp) + error_msg = get_error(file.sftp.session) + throw(LibSSHException("Read of $file failed with code $error_code: '$error_msg'")) + end + + push!(handles, (bytes_requested + 1, ret, handle)) + bytes_requested += ret + end + catch + free_handles() + rethrow() + end + + # Wait for the requests to be completed + try + for (pos, chunk_size, handle) in handles + GC.@preserve handle out begin + handle_ptr = Base.unsafe_convert(Ptr{lib.sftp_aio}, handle) + buffer_ptr = Ptr{Cvoid}(pointer(out, pos)) + ret = _session_trywait(file.sftp.session) do + @lockandblock file.sftp.session lib.sftp_aio_wait_read(handle_ptr, buffer_ptr, Csize_t(chunk_size)) + end + if ret == SSH_ERROR + throw(LibSSHException("Reading $(file) from $(pos):$(pos + chunk_size - 1) failed: $(ret)")) + end + end + end + finally + free_handles() + end + + return out +end + +""" +$(TYPEDSIGNATURES) + +Read the whole file as a `String`. +""" +Base.read(file::SftpFile, ::Type{String}) = String(read(file)) + +""" +$(TYPEDSIGNATURES) + +Write `data` to the remote file and returns the number of bytes written. This +uses libssh's asynchronous IO API so it may launch multiple parallel requests. + +# Throws +- `ArgumentError`: If `file` is closed. +- [`LibSSHException`](@ref): If writing fails. +""" +function Base.write(file::SftpFile, data::T) where T <: DenseVector + if !isopen(file) + throw(ArgumentError("$file is closed, cannot write to it")) + end + + handles = Base.RefValue{lib.sftp_aio}[] + free_handles = () -> map(x -> lib.sftp_aio_free(x[]), handles) + + # Launch requests + bytes_left = sizeof(data) + try + while bytes_left > 0 + handle = Ref{lib.sftp_aio}() + offset = length(data) - bytes_left + ret = GC.@preserve data lib.sftp_aio_begin_write(file, Ptr{Cvoid}(pointer(data)) + offset, bytes_left, handle) + if ret == SSH_ERROR + error_code = get_error(file.sftp) + error_msg = get_error(file.sftp.session) + throw(LibSSHException("Attempted write to $file failed with code $error_code: '$error_msg'")) + end + + push!(handles, handle) + bytes_left -= ret + end + catch + free_handles() + rethrow() + end + + # Wait for the requests to be completed + try + for handle in handles + GC.@preserve handle begin + handle_ptr = Base.unsafe_convert(Ptr{lib.sftp_aio}, handle) + ret = _session_trywait(file.sftp.session) do + @lockandblock file.sftp.session lib.sftp_aio_wait_write(handle_ptr) + end + if ret == SSH_ERROR + throw(LibSSHException("Write to $(file) failed: $(ret)")) + end + end + end + finally + free_handles() + end + + # At this point we should have written all the data + return sizeof(data) +end + +""" +$(TYPEDSIGNATURES) + +Write a string directly to `file`. Uses [`Base.write(::SftpFile, +::DenseVector)`](@ref) internally. +""" +Base.write(file::SftpFile, data::AbstractString) = write(file, codeunits(data)) diff --git a/test/LibSSHTests.jl b/test/LibSSHTests.jl index 2fae533..e8f0dc5 100644 --- a/test/LibSSHTests.jl +++ b/test/LibSSHTests.jl @@ -80,6 +80,22 @@ function demo_server_with_session(f::Function, port, args...; return demo_server end +function demo_server_with_sftp(f::Function, args...; kwargs...) + demo_server = demo_server_with_session(args...; kwargs...) do session + # Create an SFTP session + sftp = ssh.SftpSession(session) + + try + f(sftp) + finally + close(sftp) + end + end + + return demo_server +end + + @testset "Server" begin hostkey = joinpath(@__DIR__, "ed25519_test_key") @@ -519,6 +535,234 @@ end end end +@testset "SFTP" begin + @testset "Initialization and finalizing" begin + demo_server_with_session(2222; verbose=false) do session + # session.log_verbosity = ssh.SSH_LOG_TRACE + sftp = ssh.SftpSession(session) + + # Test state after creation + @test lib.ssh_is_blocking(session) == 0 + @test sftp.ptr isa lib.sftp_session + @test isassigned(sftp) + @test ssh.get_error(sftp) == ssh.SftpError_Ok + @test Base.unsafe_convert(lib.sftp_session, sftp) isa lib.sftp_session + + # And after closing + close(sftp) + @test isnothing(sftp.ptr) + @test !isassigned(sftp) + @test_throws ArgumentError ssh.get_error(sftp) + @test_throws ArgumentError Base.unsafe_convert(lib.sftp_session, sftp) + + # Closing twice shouldn't cause an error + close(sftp) + + # Test the finalizer + sftp = ssh.SftpSession(session) + finalize(sftp) + @test !isassigned(sftp) + + # Test the do-constructor + ssh.SftpSession(session) do sftp + @test isopen(sftp) + end + + # We shouldn't be able to create an SftpSession from a closed Session + close(session) + @test_throws ArgumentError ssh.SftpSession(session) + end + end + + @testset "Opening" begin + # Test opening + demo_server_with_sftp(2222; verbose=false) do sftp + mktempdir() do tmpdir + # Opening a file that doesn't exist should throw + bad_file = joinpath(tmpdir, "no") + @test_throws ssh.LibSSHException open(bad_file, sftp) + + # Create a dummy file + good_file = joinpath(tmpdir, "foo") + write(good_file, "foo") + + # Opening it should work + file = open(good_file, sftp) + + @test file.ptr isa lib.sftp_file + @test isassigned(file) + @test isopen(file) + @test isreadable(file) + @test isreadonly(file) + @test !iswritable(file) + + @test position(file) == 0 + seek(file, 1) + @test position(file) == 1 + + # Finalizing shouldn't actually do anything other than print an + # error because properly closing the file involves a task switch. + @test_logs (:error,) (finalize(file); flush(stdout)) + @test isopen(file) + + close(file) + @test !isassigned(file) + @test !isopen(file) + @test !isreadable(file) + @test_throws ArgumentError position(file) + @test_throws ArgumentError seek(file, 0) + + # Test append mode, which doesn't have native support + file = open(good_file, sftp; append=true) + @test position(file) == 3 + @test iswritable(file) + + # Test the do-constructor + ret = open(good_file, sftp) do file + @test isopen(file) + + 42 + end + @test ret == 42 + + # We shouldn't be able to open a file with a closed SftpSession + close(sftp) + @test_throws ArgumentError open(good_file, sftp) + end + end + end + + @testset "Reading" begin + # Test reading + demo_server_with_sftp(2222; verbose=false) do sftp + # sftp.session.log_verbosity = ssh.SSH_LOG_TRACE + mktemp() do path, io + # Test reading an empty file + file = open(path, sftp) + @test read(file) == UInt8[] + + # Read a file that's smaller than the server limit for a single + # request. + msg = "foo" + limits = ssh.get_limits(sftp) + @assert length(msg) < limits.max_read_length + write(path, msg) + + @test read(file) == collect(UInt8, msg) + @test position(file) == 3 + seekstart(file) + @test read(file, String) == msg + + # Test behaviour when we aren't at the beginning of the file + data = rand(UInt8, 10) + write(path, data) + seek(file, 5) + @test read(file) == data[6:end] + + # Test reading a file larger than the server limit for a single + # request. + data = rand(UInt8, limits.max_read_length + 1) + write(path, data) + seekstart(file) + @test read(file) == data + + # Shouldn't be able to read from a closed file + close(file) + @test_throws ArgumentError read(file) + end + end + end + + @testset "Writing" begin + # Test writing + demo_server_with_sftp(2222) do sftp + # sftp.session.log_verbosity = ssh.SSH_LOG_PROTOCOL + + mktempdir() do tmpdir + path = joinpath(tmpdir, "foo") + file = open(path, sftp; write=true, mode=0o600) + + # When we open a file for writing Base.open_flags() defaults to + # setting `create=true`, so the file should exist. + @test file.flags.create + @test isfile(path) + # Also test that the mode is set correctly + @test 0o777 & filemode(path) == 0o600 + + # Simple test + msg = "foo" + @test write(file, collect(UInt8, msg)) == 3 + @test read(path, String) == msg + + # Test writing strings directly, which should work without + # copying because we support writing DenseVector's and use + # codeunits(). + @test write(file, "foo") == 3 + # Also tests writing from somewhere other than the beginning of the file + @test read(path, String) == "foofoo" + + # Test writing data larger than the server limit for a single request + limits = ssh.get_limits(sftp) + data = rand(UInt8, limits.max_write_length + 1) + seekstart(file) + @test write(file, data) == length(data) + @test read(path) == data + + # Shouldn't be able to write to a closed file + close(file) + @test_throws ArgumentError write(file, data) + end + end + end + + @testset "Misc" begin + demo_server_with_sftp(2222; verbose=false) do sftp + # Extensions + @test ssh.get_extensions(sftp) isa Dict + @test !isempty(ssh.get_extensions(sftp)) + + @test ssh.get_limits(sftp).max_read_length > 0 + + # Unfortunately our demo server doesn't support the home-directory + # extension. + @test_throws ErrorException homedir(sftp) + + mktemp() do path, io + # stat()'ing a non-existent file should fail + @test_throws ssh.LibSSHException stat(path * "_bad", sftp) + + attrs = stat(path, sftp) + @test isassigned(attrs) + @test attrs.size == 0 + + # Smoke test for Base.show() + show(IOBuffer(), attrs) + + write(io, "foo") + flush(io) + + attrs = stat(path, sftp) + @test attrs.size == 3 + + # Not entirely sure why, but the demo server won't return any strings + @test attrs.name == "" + @test attrs.extended_type == "" + + # Test the finalizer + finalize(attrs) + @test !isassigned(attrs) + end + + close(sftp) + + @test_throws ArgumentError stat("/tmp", sftp) + @test_throws ArgumentError ssh.get_extensions(sftp) + @test_throws ArgumentError ssh.get_limits(sftp) + @test_throws ArgumentError homedir(sftp) + end + end +end + @testset "PKI" begin rsa = pki.generate(pki.KeyType_rsa) @test pki.key_type(rsa) == pki.KeyType_rsa From 29bc60edab62459a7691625f529138e6d193ac24 Mon Sep 17 00:00:00 2001 From: JamesWrigley Date: Sun, 6 Oct 2024 21:02:07 +0200 Subject: [PATCH 11/11] Temporarily disable the safe part of _safe_poll_fd() It seems to be causing something to spin. Should be replaced with a different design anyway. --- src/LibSSH.jl | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/LibSSH.jl b/src/LibSSH.jl index 77cd840..ef527c5 100644 --- a/src/LibSSH.jl +++ b/src/LibSSH.jl @@ -162,16 +162,19 @@ end # versions between the loop condition evaluation and this line, so we wrap # poll_fd() in a try-catch in case the bind (and thus the file descriptor) has # been closed in the meantime, which would cause poll_fd() to throw an IOError: -# https://github.com/JuliaLang/julia/pull/52377 +# https://github.com/JuliaLang/julia/pull/52377. +# +# Note: the whole polling design is bad. We should only have one task polling +# the session and waking up other listeners. function _safe_poll_fd(args...; kwargs...) result = nothing - try + # try result = FileWatching.poll_fd(args...; kwargs...) - catch ex - if !(ex isa Base.IOError) - rethrow() - end - end + # catch ex + # if !(ex isa Base.IOError) + # rethrow() + # end + # end return result end