Merge pull request #65 from JuliaIO/sf/chaos_buffer_test

Author: staticfloat

Project.toml

@@ -17,6 +17,7 @@ Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
 Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c"
 Tar_jll = "9b64493d-8859-5bf3-93d7-7c32dd38186f"
 Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
+SimpleBufferStream = "777ac1f9-54b0-4bf8-805c-2214025038e7"
 
 [targets]
-test = ["Pkg", "Random", "Tar_jll", "Test"]
+test = ["Pkg", "Random", "Tar_jll", "Test", "SimpleBufferStream"]

src/extract.jl

@@ -250,17 +250,17 @@ function git_file_hash(
     # TODO: this largely duplicates the logic of read_data
     # read_data could be used directly if SHA offered an interface
     # where you write data to an IO object and it maintains a hash
-    t = round_up(size)
-    while size > 0
-        n = min(t, length(buf))
-        r = readbytes!(tar, buf, n)
-        r < n && eof(tar) && throw(EOFError())
-        v = view(buf, 1:min(r, size))
-        SHA.update!(ctx, v)
-        size -= length(v)
-        t -= r
+    padded_size = round_up(size)
+    while padded_size > 0
+        max_read_len = min(padded_size, length(buf))
+        read_len = readbytes!(tar, buf, max_read_len)
+        read_len < max_read_len && eof(tar) && throw(EOFError())
+        nonpadded_view = view(buf, 1:min(read_len, size))
+        SHA.update!(ctx, nonpadded_view)
+        size -= length(nonpadded_view)
+        padded_size -= read_len
     end
-    @assert size == t == 0
+    @assert size == padded_size == 0
     return bytes2hex(SHA.digest!(ctx))
 end
 
@@ -516,16 +516,16 @@ function read_data(
     buf::Vector{UInt8} = Vector{UInt8}(undef, DEFAULT_BUFFER_SIZE),
     tee::IO = devnull,
 )::Nothing
-    t = round_up(size)
-    while size > 0
-        n = min(t, length(buf))
-        r = readbytes!(tar, buf, n)
-        write(tee, view(buf, 1:r))
-        r < n && eof(io) && error("premature end of tar file")
-        size -= write(file, view(buf, 1:min(r, size)))
-        t -= r
+    padded_size = round_up(size)
+    while padded_size > 0
+        max_read_len = min(padded_size, length(buf))
+        read_len = readbytes!(tar, buf, max_read_len)
+        write(tee, view(buf, 1:read_len))
+        read_len < max_read_len && eof(tar) && error("premature end of tar file")
+        size -= write(file, view(buf, 1:min(read_len, size)))
+        padded_size -= read_len
     end
-    @assert size == t == 0
+    @assert size == padded_size == 0
     return
 end
 

test/runtests.jl

@@ -1,5 +1,43 @@
 include("setup.jl")
 
+@testset "ChaosBufferStream" begin
+    @testset "constant usage" begin
+        io = BufferStream()
+        cio = ChaosBufferStream(io; chunksizes=[17], sleepamnts=[0.001])
+        write(io, rand(UInt8, 30))
+        close(io)
+
+        # Test that data comes out in 17-byte chunks (except for the last)
+        buff = Array{UInt8}(undef, 30)
+        t = @elapsed begin
+            @test readbytes!(cio, buff, 30) == 17
+            @test readbytes!(cio, buff, 30) == 13
+        end
+        @test t >= 0.001
+    end
+
+    @testset "random usage" begin
+        io = BufferStream()
+        chunksizes = 5:10
+        cio = ChaosBufferStream(io; chunksizes=chunksizes, sleepamnts=[0.0])
+        write(io, rand(UInt8, 3000))
+        close(io)
+
+        buff = Array{UInt8}(undef, 10)
+        while !eof(cio)
+            r = readbytes!(cio, buff, 10)
+            # In normal operation, the chunk size must be one of
+            # the given chunksizes, but at the end of the stream
+            # it is allowed to be less.
+            if !eof(cio)
+                @test r ∈ chunksizes
+            else
+                @test r <= maximum(chunksizes)
+            end
+        end
+    end
+end
+
 @testset "empty tarball" begin
     dir = mktempdir()
     tarball = Tar.create(dir)
@@ -430,6 +468,32 @@ end
         end
     end
 
+    @testset "inconvenient stream buffering" begin
+        # We will try feeding in an adversarial length that used to cause an assertion error
+        open(tarball, read=true) do io
+            # This will cause an assertion error because we know the padded space beyond the
+            # end of the test file content will be larger than 17 bytes, causing the `for`
+            # loop to exit early, failing the assertion.
+            @test hash == Tar.tree_hash(ChaosBufferStream(io; chunksizes=[17]); skip_empty=true)
+        end
+
+        # This also affected read_data()
+        mktempdir() do dir
+            open(tarball, read=true) do io
+                Tar.extract(ChaosBufferStream(io; chunksizes=[17]), dir)
+                check_tree_hash(hash, dir)
+            end
+        end
+
+        # We also perform a fuzzing test to convince ourselves there are no other errors
+        # of this type within `Tar.tree_hash()`.
+        for idx in 1:100
+            open(tarball, read=true) do io
+                @test hash == Tar.tree_hash(ChaosBufferStream(io), skip_empty=true)
+            end
+        end
+    end
+
     @testset "with predicate" begin
         # generate a version of dir with .skip entries
         dir = make_test_dir(true)

test/setup.jl

@@ -1,6 +1,7 @@
 using Test
 using Random
 using ArgTools
+using SimpleBufferStream
 
 if !Sys.iswindows()
     using Tar_jll
@@ -104,7 +105,13 @@ function make_test_dir(gen_skip::Bool=false)
         touch(joinpath(dir, "file.skip"))
     end
     mkdir(joinpath(dir, "dir"))
-    touch(joinpath(dir, "dir", "file"))
+
+    # Create a file that actually has content
+    fpath = joinpath(dir, "dir", "file")
+    touch(fpath)
+    open(fpath, write=true) do io
+        write(io, rand(UInt8, 1000))
+    end
     mkdir(joinpath(dir, "empty"))
     if gen_skip
         touch(joinpath(dir, "dir", "file.skip"))
@@ -147,3 +154,33 @@ end
 function tar_write_dir(io::IO, path::String, mode::Integer=0o755)
     Tar.write_header(io, Tar.Header(path, :directory, mode, 0, ""))
 end
+
+"""
+    ChaosBufferStream(input::IO; chunksizes, sleepamnts)
+
+Acts as a highly-inconvenient BufferStream, allowing tests to purposefully break
+up their data flow reads/writes into chunks of random size (taken via `rand(chunksizes)`)
+and with a pause of `rand(sleepamnts)` in between each chunk.
+
+Usage example:
+
+    t_hash = Tar.tree_hash(ChaosBufferStream(io))
+
+Or slightly more complicated:
+
+    http_io = BufferStream()
+    @async HTTP.get(url; response_stream=http_io)
+    t_hash = Tar.tree_hash(ChaosBufferStream(http_io; chunksizes=4000:8000, sleepamnts=[1e-3, 2e-3]))
+"""
+function ChaosBufferStream(input::IO; chunksizes = 1024:2048, sleepamnts = 1e-3:1e-4:5e-3)
+    output = BufferStream()
+    @async begin
+        while !eof(input)
+            sleep(rand(sleepamnts))
+            chunk = read(input, rand(chunksizes))
+            write(output, chunk)
+        end
+        close(output)
+    end
+    return output
+end