shrine: [Bug]: Corruption of downloaded files when using `net-protocol v0.2.0`

Report

There is a problem with corruption when downloading files that have been uploaded to S3. We have not been able to identify the types of files that reproduce, but PDF files have a high probability of reproduction. Corrupted PDF files will not be able to be opened with common PDF Viewer.

Expected Behavior

You can download the same file you uploaded.

Actual Behavior

A corrupted file that is different from the uploaded file will be downloaded.

Steps to Reproduce the Problem

# Upload some file.
original_file = File.open('sample.pdf')
uploaded_file = Shrine.upload(original_file, :store)

# Download that file.
download_file = uploaded_file.download

# Differing file contents.
File.read(original_file) == File.read(download_file)

Ruby Version

Ruby 3.1.2

Shrine Version

3.4.0

Anything else?

https://github.com/shrinerb/shrine/issues/609 as well as net-protocol v0.2.0 I think this is an effect of https://github.com/ruby/net-protocol/commit/781e400389e75284327d97a3185d9c05537f2611.

AWS S3 is used for storage. Other storage has not been tested.

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 16 (9 by maintainers)

Commits related to this issue

Undo BufferedIO#rbuf_consume_all_shareable! optimization This optimization is unsafe because `dest` is allowed to be a custom object responding to `<<` (e.g. a block wrapped in `ReadAdapter`). So th... — committed to casperisfine/net-protocol by byroot 2 years ago
Undo BufferedIO#rbuf_consume_all_shareable! optimization This optimization is unsafe because `dest` is allowed to be a custom object responding to `<<` (e.g. a block wrapped in `ReadAdapter`). So th... — committed to casperisfine/net-protocol by byroot 2 years ago
[ruby/net-protocol] Undo BufferedIO#rbuf_consume_all_shareable! optimization This optimization is unsafe because `dest` is allowed to be a custom object responding to `<<` (e.g. a block wrapped in `R... — committed to ruby/ruby by byroot 2 years ago

Most upvoted comments

So I have a fix for this in net-protocol, I’ll make sure a new version is cut ASAP and that Ruby 3.2.0 ships with a working version: https://github.com/ruby/net-protocol/pull/19

casperisfine on Dec 7, 2022

So, I don’t see a place where we can warn about this

You could warn when instantiating the S3 store. e.g.: warn "blah blah" if Net::Protocol::VERSION == "0.2.0".

casperisfine on Dec 8, 2022

Thank you very much @casperisfine and @janko , I appreciate your work!

jrochkind on Dec 8, 2022

Ok, I managed to repro using fakes3 and a monkey patch:

require 'shrine'
require 'shrine/storage/s3'

module AWSPatch
  def start_session(endpoint)
    if endpoint == "https://shrine-debug.s3.us-east1.amazonaws.com"
      super("http://localhost:4567") # using fakes3
    else
      super
    end
  end
end

Seahorse::Client::NetHttp::ConnectionPool.prepend(AWSPatch)

Shrine.storages = {
  store: Shrine::Storage::S3.new(
    bucket: "shrine-debug",
    region: "us-east1",
    access_key_id: "FAKE",
    secret_access_key: "FAKE",
  ),
}
original_file = File.open('/Users/byroot/Downloads/protocol-repro.pdf')
uploaded_file = Shrine.upload(original_file, :store)

# Download that file.
download_file = uploaded_file.download

# Differing file contents.
p File.read(original_file) == File.read(download_file)

I’ll dig more. And thanks for the pointers @janko. If necessary we’ll revert the net-protocol changes, but hopefully we find a way to fix this without losing the extra perf.

casperisfine on Dec 7, 2022

👋 author of https://github.com/ruby/net-protocol/commit/781e400389e75284327d97a3185d9c05537f2611 here. I’d like to reproduce to investigate but help would be welcome.

Is there a way to reproduce that doesn’t require AWS credentials?

casperisfine on Dec 7, 2022