Avoid using String constructor on Vector{UInt8} (#52)

* avoid using String on vector of bytes

* add tests

src/filename-checks.jl

@@ -55,7 +55,7 @@ function check_name_used(name::String, used_names::Set{String}, used_stripped_di
         # need to rstrip because of repeated '/'
         if isempty(parent_data) || parent_data[end] != UInt8('/')
             # this effectively rstrips all '/'
-            parent_name = String(parent_data)
+            parent_name = copy_string(parent_data)
             # @show parent_name
             @argcheck parent_name ∉ used_names
         end
@@ -75,7 +75,7 @@ function add_name_used!(name::String, used_names::Set{String}, used_stripped_dir
         # need to rstrip because of repeated '/'
         if isempty(parent_data) || parent_data[end] != UInt8('/')
             # this effectively rstrips all '/'
-            parent_name = String(parent_data)
+            parent_name = copy_string(parent_data)
             # @show parent_name
             push!(used_stripped_dir_names, parent_name)
         end     

src/reader.jl

@@ -12,6 +12,14 @@ const ByteArray = Union{
     Base.FastContiguousSubArray{UInt8,1,Vector{UInt8}}
 }
 
+# version of String(v::AbstractVector{UInt8}) that works consistently.
+function copy_string(v::AbstractVector{UInt8})::String
+    String(v)
+end
+function copy_string(v::Vector{UInt8})::String
+    GC.@preserve v unsafe_string(pointer(v), length(v))
+end
+
 """
     zip_crc32(data::AbstractVector{UInt8}, crc::UInt32=UInt32(0))::UInt32
 
@@ -64,7 +72,7 @@ Return the name of entry `i`.
 
 `i` can range from `1:zip_nentries(x)`
 """
-zip_name(x::HasEntries, i::Integer)::String = String(_name_view(x, i))
+zip_name(x::HasEntries, i::Integer)::String = copy_string(_name_view(x, i))
 
 """
     zip_names(x::HasEntries)::Vector{String}
@@ -103,7 +111,7 @@ zip_iscompressed(x::HasEntries, i::Integer)::Bool = x.entries[i].method != Store
 
 Return the comment attached to entry `i`
 """
-zip_comment(x::HasEntries, i::Integer)::String = String(view(x.central_dir_buffer, x.entries[i].comment_range))
+zip_comment(x::HasEntries, i::Integer)::String = copy_string(view(x.central_dir_buffer, x.entries[i].comment_range))
 
 """
     zip_stored_crc32(x::HasEntries, i::Integer)::UInt32
@@ -293,7 +301,7 @@ function zip_readentry(r::ZipReader, s::AbstractString)
 end
 
 function zip_readentry(r::ZipReader, i::Union{AbstractString, Integer}, ::Type{String})
-    String(zip_readentry(r, i))
+    copy_string(zip_readentry(r, i))
 end
 
 

src/writer.jl

@@ -100,7 +100,7 @@ function zip_append_archive(io::IO; trunc_footer=true, zip_kwargs=(;))::ZipWrite
         w.central_dir_buffer = central_dir_buffer
         if w.check_names
             for e in entries
-                add_name_used!(String(view(central_dir_buffer, e.name_range)), w.used_names, w.used_stripped_dir_names)
+                add_name_used!(copy_string(view(central_dir_buffer, e.name_range)), w.used_names, w.used_stripped_dir_names)
             end
         end
         w

test/runtests.jl

@@ -9,11 +9,12 @@ Random.seed!(1234)
 
 
 # @test Any[] == detect_ambiguities(Base, Core, ZipArchives)
+include("test_copy_string.jl")
 include("test_simple-usage.jl")
 include("test_filename-checks.jl")
 include("test_show.jl")
 include("test_writer.jl")
 include("test_reader.jl")
 include("test_appending.jl")
 include("test_ported-go-tests.jl")
-include("test_big_zips.jl")
+include("test_big_zips.jl")

test/test_copy_string.jl

@@ -0,0 +1,59 @@
+include("common.jl")
+using OffsetArrays: Origin
+
+@testset "copy_string" begin
+    # copy_string is an internal function
+
+    a = UInt8[]
+    s = ZipArchives.copy_string(a)
+    @test s == ""
+    @test a == UInt8[]
+    push!(a, 0x61)
+    @test s == ""
+    s = ZipArchives.copy_string(a)
+    @test s == "a"
+    @test a == [0x61]
+
+    a = UInt8[0x00]
+    s = ZipArchives.copy_string(a)
+    @test s == "\0"
+    @test a == UInt8[0x00]
+    push!(a, 0x61)
+    @test s == "\0"
+    s = ZipArchives.copy_string(a)
+    @test s == "\0a"
+    @test a == [0x00, 0x61]
+
+    a = UInt8[0x00]
+    s = ZipArchives.copy_string(a)
+    @test s == "\0"
+    @test a == UInt8[0x00]
+    pushfirst!(a, 0x61)
+    @test s == "\0"
+    s = ZipArchives.copy_string(a)
+    @test s == "a\0"
+    @test a == [0x61, 0x00]
+
+    a = Origin(0)([0x61,0x62,0x63])
+    b = ZipArchives.copy_string(a)
+    @test a == Origin(0)([0x61,0x62,0x63])
+    @test b == "abc"
+    a[0] = 0x62
+    @test b == "abc"
+
+    io = IOBuffer()
+    write(io, [0x61,0x62,0x63])
+    seekstart(io)
+    a = read(io, 3)
+    @test a == [0x61,0x62,0x63]
+    b = reshape(a, 1, 3)
+    c = reshape(b, 3)
+    s = ZipArchives.copy_string(a)
+    @test a == [0x61,0x62,0x63]
+    @test c == [0x61,0x62,0x63]
+    @test s == "abc"
+    c[1] = 0x62
+    @test a == [0x62,0x62,0x63]
+    @test c == [0x62,0x62,0x63]
+    @test s == "abc"
+end