Merge branch 'fix-deadlock-chunked-io' into 'master'

Fix deadlock on ChunkedIO

See merge request gitlab-org/gitlab-ce!23329

changelogs/unreleased/fix-deadlock-chunked-io.yml

+---
+title: Fix deadlock on ChunkedIO
+merge_request:
+author:
+type: fixed

lib/gitlab/ci/trace/chunked_io.rb

@@ -13,7 +13,7 @@ module Gitlab
         attr_reader :build
         attr_reader :tell, :size
-        attr_reader :chunk, :chunk_range
+        attr_reader :chunk_data, :chunk_range
         alias_method :pos, :tell
@@ -75,14 +75,14 @@ module Gitlab
           until length <= 0 || eof?
             data = chunk_slice_from_offset
-            break if data.empty?
+            raise FailedToGetChunkError if data.empty?
             chunk_bytes = [CHUNK_SIZE - chunk_offset, length].min
-            chunk_data = data.byteslice(0, chunk_bytes)
-            out << chunk_data
-            @tell += chunk_data.bytesize
-            length -= chunk_data.bytesize
+            chunk_data_slice = data.byteslice(0, chunk_bytes)
+            out << chunk_data_slice
+            @tell += chunk_data_slice.bytesize
+            length -= chunk_data_slice.bytesize
           end
           out = out.join
@@ -100,11 +100,14 @@ module Gitlab
           until eof?
             data = chunk_slice_from_offset
+            raise FailedToGetChunkError if data.empty?
+
             new_line = data.index("\n")
             if !new_line.nil?
-              out << data[0..new_line]
-              @tell += new_line + 1
+              raw_data = data[0..new_line]
+              out << raw_data
+              @tell += raw_data.bytesize
               break
             else
               out << data
@@ -121,13 +124,13 @@ module Gitlab
           while tell < start_pos + data.bytesize
             # get slice from current offset till the end where it falls into chunk
             chunk_bytes = CHUNK_SIZE - chunk_offset
-            chunk_data = data.byteslice(tell - start_pos, chunk_bytes)
+            data_slice = data.byteslice(tell - start_pos, chunk_bytes)
             # append data to chunk, overwriting from that point
-            ensure_chunk.append(chunk_data, chunk_offset)
+            ensure_chunk.append(data_slice, chunk_offset)
             # move offsets within buffer
-            @tell += chunk_data.bytesize
+            @tell += data_slice.bytesize
             @size = [size, tell].max
           end
@@ -183,12 +186,12 @@ module Gitlab
             current_chunk.tap do |chunk|
               raise FailedToGetChunkError unless chunk
-              @chunk = chunk.data
+              @chunk_data = chunk.data
               @chunk_range = chunk.range
             end
           end
-          @chunk[chunk_offset..CHUNK_SIZE]
+          @chunk_data.byteslice(chunk_offset, CHUNK_SIZE)
         end
         def chunk_offset

lib/gitlab/http_io.rb

@@ -85,11 +85,11 @@ module Gitlab
         break if data.empty?
         chunk_bytes = [BUFFER_SIZE - chunk_offset, length].min
-        chunk_data = data.byteslice(0, chunk_bytes)
-        out << chunk_data
-        @tell += chunk_data.bytesize
-        length -= chunk_data.bytesize
+        data_slice = data.byteslice(0, chunk_bytes)
+        out << data_slice
+        @tell += data_slice.bytesize
+        length -= data_slice.bytesize
       end
       out = out.join

spec/lib/gitlab/ci/trace/chunked_io_spec.rb

@@ -116,6 +116,19 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
         chunked_io.each_line { |line| }
       end
     end
+
+    context 'when buffer consist of many empty lines' do
+      let(:sample_trace_raw) { Array.new(10, "   ").join("\n") }
+
+      before do
+        build.trace.set(sample_trace_raw)
+      end
+
+      it 'yields lines' do
+        expect { |b| chunked_io.each_line(&b) }
+          .to yield_successive_args(*string_io.each_line.to_a)
+      end
+    end
   end
   context "#read" do
@@ -143,6 +156,22 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
       end
     end
+    context 'when chunk is missing data' do
+      let(:length) { nil }
+
+      before do
+        stub_buffer_size(1024)
+        build.trace.set(sample_trace_raw)
+
+        # make second chunk to not have data
+        build.trace_chunks.second.append('', 0)
+      end
+
+      it 'raises an error' do
+        expect { subject }.to raise_error described_class::FailedToGetChunkError
+      end
+    end
+
     context 'when read only first 100 bytes' do
       let(:length) { 100 }
@@ -266,6 +295,40 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do
         expect(chunked_io.readline).to eq(string_io.readline)
       end
     end
+
+    context 'when chunk is missing data' do
+      let(:length) { nil }
+
+      before do
+        build.trace.set(sample_trace_raw)
+
+        # make first chunk to have invalid data
+        build.trace_chunks.first.append('data', 0)
+      end
+
+      it 'raises an error' do
+        expect { subject }.to raise_error described_class::FailedToGetChunkError
+      end
+    end
+
+    context 'when utf-8 is being used' do
+      let(:sample_trace_raw) { sample_trace_raw_utf8.force_encoding(Encoding::BINARY) }
+      let(:sample_trace_raw_utf8) { "😺\n😺\n😺\n😺" }
+
+      before do
+        stub_buffer_size(3) # the utf-8 character has 4 bytes
+
+        build.trace.set(sample_trace_raw_utf8)
+      end
+
+      it 'has known length' do
+        expect(sample_trace_raw_utf8.bytesize).to eq(4 * 4 + 3 * 1)
+        expect(sample_trace_raw.bytesize).to eq(4 * 4 + 3 * 1)
+        expect(chunked_io.size).to eq(4 * 4 + 3 * 1)
+      end
+
+      it_behaves_like 'all line matching'
+    end
   end
   context "#write" do