clipboard,win32: rewrite clipboard functions to handle errors

Per comment (although it failed to note that we were previously always leaking the memory).

Author: vtjnash

stdlib/InteractiveUtils/src/clipboard.jl

@@ -18,6 +18,7 @@ if Sys.isapple()
         open(pipeline(pbcopy_cmd, stderr=stderr), "w") do io
             print(io, x)
         end
+        nothing
     end
     function clipboard()
         pbpaste_cmd = `pbpaste`
@@ -26,89 +27,113 @@ if Sys.isapple()
         if Sys.which("reattach-to-user-namespace") != nothing
             pbcopy_cmd = `reattach-to-user-namespace pbpaste`
         end
-        read(pbpaste_cmd, String)
+        return read(pbpaste_cmd, String)
     end
 
 elseif Sys.islinux() || Sys.KERNEL === :FreeBSD
     _clipboardcmd = nothing
-    const _clipboardcmds = Dict(
-        :copy => Dict(
+    const _clipboard_copy = Dict(
             :xsel  => Sys.islinux() ?
-                `xsel --nodetach --input --clipboard` : `xsel -c`,
+                `xsel --nodetach --input --clipboard` :
+                `xsel -c`,
             :xclip => `xclip -silent -in -selection clipboard`,
-        ),
-        :paste => Dict(
+        )
+    const _clipboard_paste = Dict(
             :xsel  => Sys.islinux() ?
-                `xsel --nodetach --output --clipboard` : `xsel -p`,
+                `xsel --nodetach --output --clipboard` :
+                `xsel -p`,
             :xclip => `xclip -quiet -out -selection clipboard`,
         )
-    )
     function clipboardcmd()
         global _clipboardcmd
         _clipboardcmd !== nothing && return _clipboardcmd
         for cmd in (:xclip, :xsel)
             success(pipeline(`which $cmd`, devnull)) && return _clipboardcmd = cmd
         end
-        pkgs = @static if Sys.islinux()
-            "xsel or xclip"
-        elseif Sys.KERNEL === :FreeBSD
+        pkgs = @static if Sys.KERNEL === :FreeBSD
             "x11/xsel or x11/xclip"
+        else
+            "xsel or xclip"
         end
         error("no clipboard command found, please install $pkgs")
     end
     function clipboard(x)
         c = clipboardcmd()
-        cmd = get(_clipboardcmds[:copy], c, nothing)
-        if cmd === nothing
-            error("unexpected clipboard command: $c")
-        end
+        cmd = _clipboard_copy[c]
         open(pipeline(cmd, stderr=stderr), "w") do io
             print(io, x)
         end
+        nothing
     end
     function clipboard()
         c = clipboardcmd()
-        cmd = get(_clipboardcmds[:paste], c, nothing)
-        if cmd === nothing
-            error("unexpected clipboard command: $c")
-        end
-        read(pipeline(cmd, stderr=stderr), String)
+        cmd = _clipboardcmds_paste[c]
+        return read(pipeline(cmd, stderr=stderr), String)
     end
 
 elseif Sys.iswindows()
-    # TODO: these functions leak memory and memory locks if they throw an error
     function clipboard(x::AbstractString)
         if Base.containsnul(x)
             throw(ArgumentError("Windows clipboard strings cannot contain NUL character"))
         end
-        Base.windowserror(:OpenClipboard, 0==ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), C_NULL))
-        Base.windowserror(:EmptyClipboard, 0==ccall((:EmptyClipboard, "user32"), stdcall, Cint, ()))
         x_u16 = Base.cwstring(x)
+        pdata = Ptr{UInt16}(C_NULL)
+        function cleanup(cause)
+            errno = cause == :success ? UInt32(0) : Libc.GetLastError()
+            if cause !== :OpenClipboard
+                if cause !== :success && pdata != C_NULL
+                    ccall((:GlobalFree, "kernel32"), stdcall, Cint, (Ptr{UInt16},), pdata)
+                end
+                ccall((:CloseClipboard, "user32"), stdcall, Cint, ()) == 0 && Base.windowserror(:CloseClipboard) # this should never fail
+            end
+            cause == :success || Base.windowserror(cause, errno)
+            nothing
+        end
+        ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), C_NULL) == 0 && return Base.windowserror(:OpenClipboard)
+        ccall((:EmptyClipboard, "user32"), stdcall, Cint, ()) == 0 && return cleanup(:EmptyClipboard)
         # copy data to locked, allocated space
-        p = ccall((:GlobalAlloc, "kernel32"), stdcall, Ptr{UInt16}, (UInt16, Int32), 2, sizeof(x_u16))
-        Base.windowserror(:GlobalAlloc, p==C_NULL)
-        plock = ccall((:GlobalLock, "kernel32"), stdcall, Ptr{UInt16}, (Ptr{UInt16},), p)
-        Base.windowserror(:GlobalLock, plock==C_NULL)
-        ccall(:memcpy, Ptr{UInt16}, (Ptr{UInt16},Ptr{UInt16},Int), plock, x_u16, sizeof(x_u16))
-        Base.windowserror(:GlobalUnlock, 0==ccall((:GlobalUnlock, "kernel32"), stdcall, Cint, (Ptr{Cvoid},), plock))
-        pdata = ccall((:SetClipboardData, "user32"), stdcall, Ptr{UInt16}, (UInt32, Ptr{UInt16}), 13, p)
-        Base.windowserror(:SetClipboardData, pdata!=p)
-        ccall((:CloseClipboard, "user32"), stdcall, Cvoid, ())
+        pdata = ccall((:GlobalAlloc, "kernel32"), stdcall, Ptr{UInt16}, (UInt16, Int32), 2 #=GMEM_MOVEABLE=#, sizeof(x_u16))
+        pdata == C_NULL && return cleanup(:GlobalAlloc)
+        plock = ccall((:GlobalLock, "kernel32"), stdcall, Ptr{UInt16}, (Ptr{UInt16},), pdata)
+        plock == C_NULL && return cleanup(:GlobalLock)
+        ccall(:memcpy, Ptr{UInt16}, (Ptr{UInt16}, Ptr{UInt16}, Int), plock, x_u16, sizeof(x_u16))
+        unlock = ccall((:GlobalUnlock, "kernel32"), stdcall, Cint, (Ptr{UInt16},), pdata)
+        (unlock == 0 && Libc.GetLastError() == 0) || return cleanup(:GlobalUnlock) # this should never fail
+        pset = ccall((:SetClipboardData, "user32"), stdcall, Ptr{UInt16}, (UInt32, Ptr{UInt16}), 13, pdata)
+        pdata != pset && return cleanup(:SetClipboardData)
+        cleanup(:success)
     end
     clipboard(x) = clipboard(sprint(print, x)::String)
     function clipboard()
-        Base.windowserror(:OpenClipboard, 0==ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), C_NULL))
+        function cleanup(cause)
+            errno = cause == :success ? UInt32(0) : Libc.GetLastError()
+            if cause !== :OpenClipboard
+                ccall((:CloseClipboard, "user32"), stdcall, Cint, ()) == 0 && Base.windowserror(:CloseClipboard) # this should never fail
+            end
+            if cause !== :success && (cause !== :GetClipboardData || errno != 0)
+                Base.windowserror(cause, errno)
+            end
+            ""
+        end
+        ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), C_NULL) == 0 && return Base.windowserror(:OpenClipboard)
+        ccall(:SetLastError, stdcall, Cvoid, (UInt32,), 0) # allow distinguishing if the clipboard simply didn't have text
         pdata = ccall((:GetClipboardData, "user32"), stdcall, Ptr{UInt16}, (UInt32,), 13)
-        Base.windowserror(:GetClipboardData, pdata==C_NULL)
-        Base.windowserror(:CloseClipboard, 0==ccall((:CloseClipboard, "user32"), stdcall, Cint, ()))
+        pdata == C_NULL && return cleanup(:GetClipboardData)
         plock = ccall((:GlobalLock, "kernel32"), stdcall, Ptr{UInt16}, (Ptr{UInt16},), pdata)
-        Base.windowserror(:GlobalLock, plock==C_NULL)
-        # find NUL terminator (0x0000 16-bit code unit)
-        len = 0
-        while unsafe_load(plock, len+1) != 0; len += 1; end
-        # get Vector{UInt16}, transcode data to UTF-8, make a String of it
-        s = transcode(String, unsafe_wrap(Array, plock, len))
-        Base.windowserror(:GlobalUnlock, 0==ccall((:GlobalUnlock, "kernel32"), stdcall, Cint, (Ptr{UInt16},), plock))
+        plock == C_NULL && return cleanup(:GlobalLock)
+        s = try
+            # find NUL terminator (0x0000 16-bit code unit)
+            len = 0
+            while unsafe_load(plock, len + 1) != 0
+                len += 1
+            end
+            # get Vector{UInt16}, transcode data to UTF-8, make a String of it
+            transcode(String, unsafe_wrap(Array, plock, len))
+        finally
+           unlock = ccall((:GlobalUnlock, "kernel32"), stdcall, Cint, (Ptr{UInt16},), pdata)
+           (unlock != 0 || Libc.GetLastError() == 0) || return cleanup(:GlobalUnlock) # this should never fail
+           cleanup(:success)
+        end
         return s
     end
 

stdlib/InteractiveUtils/test/runtests.jl

@@ -383,6 +383,20 @@ withenv("JULIA_EDITOR" => nothing, "VISUAL" => nothing, "EDITOR" => nothing) do
 end
 
 # clipboard functionality
+@static if Sys.iswindows()
+    # concurrent access error
+    hDesktop = ccall((:GetDesktopWindow, "user32"), stdcall, Ptr{Cvoid}, ())
+    @test ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), hDesktop) != 0
+    @test_throws Base.SystemError("OpenClipboard", 0, Base.WindowsErrorInfo(0x00000005, nothing)) clipboard() # ACCESS_DENIED
+    @test ccall((:CloseClipboard, "user32"), stdcall, Cint, ()) != 0
+    # empty clipboard failure
+    @test ccall((:OpenClipboard, "user32"), stdcall, Cint, (Ptr{Cvoid},), C_NULL) != 0
+    @test ccall((:EmptyClipboard, "user32"), stdcall, Cint, ()) != 0
+    @test ccall((:CloseClipboard, "user32"), stdcall, Cint, ()) != 0
+    @test clipboard() == ""
+    # nul error (unsupported data)
+    @test_throws ArgumentError("Windows clipboard strings cannot contain NUL character") clipboard("abc\0")
+end
 if Sys.iswindows() || Sys.isapple()
     for str in ("Hello, world.", "∀ x ∃ y", "")
         clipboard(str)