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

[casperisfine]

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: ruby/net-protocol#19

[casperisfine]

👋 author of ruby/net-protocol@781e400 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]

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.

[jrochkind]

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

[casperisfine]

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".

[jrochkind]

I have this too after upgrading to net-protocol 0.2.0. Pinning to net-protocol "< 0.2.0"" resolved problem for me (for now, until it's fixed otherwise).

(You say 0.2.2, but I think net-protocol 0.2.0 released Dec 5 is actually the latest release of net-protocol?)

It is a tricky one, because there are no exceptions, just corrupted files.

[tmimura39]

You say 0.2.2, but I think net-protocol 0.2.0 released Dec 5 is actually the latest release of net-protocol?

oh, sorry.
That's right. My mistake in stating this.
I have corrected the description.

Thanks.

[tmimura39]

Thank you casperisfine 👍

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

Sorry, I have currently only found a way to reproduce this using AWS S3.
We will report additional information as soon as we find other simple ways to reproduce it.

[janko]

@casperisfine You can see #609 (comment) where I identified which change was most likely breaking for Shrine. TL;DR: Down::ChunkedIO that Shrine's S3 uses relies on streamed chunks to all be different string objects, but in net-protocol 0.2.0 they're reused.

[casperisfine]

Hum, actually I think fakes3 just wasn't working correctly, now using s3rver it works. That said I don't go through TLS which might be part of why it doesn't repro.

I guess I'll have to setup some S3 bucket.

[janko]

I will investigate how difficult it would be to change Down::ChunkedIO to handle chunks objects being reused. That's probably something I shouldn't have relied on in the first place 🙂

[janko]

Thanks a lot @casperisfine!

[janko]

Version 0.2.1 of net-protocol was just released with the fix by @casperisfine, which resolves this issue 🤘🏻

[jrochkind]

I wonder if shrine (or down) should add "net-protocol", "!= 0.2.0" to it's dependencies, to save people the confusion of accidentally using the buggy release, especially because it ships with ruby 3.1. (It's annoying because a "corrupt bytes" problem is in my experience especially unlikely to be covered by one's tests in depending project, and is confusing to debug).

But of course that would only apply to new shrine/down release anyway, so might just make bundle update more confusing, not sure. Just thought I'd bring it up for consideration.

[janko]

especially because it ships with ruby 3.1

According to stdgems.org, the version of net-protocol shipped with Ruby 3.1 is 0.1.2.

But of course that would only apply to new shrine/down release anyway, so might just make bundle update more confusing, not sure. Just thought I'd bring it up for consideration.

Yeah, that was my initial thought as well, but Shrine technically doesn't depend on net-protocol, in this case it was aws-sdk-s3 being used in tandem with Down, where aws-sdk-s3's usage of net-http (and thus net-protocol) is an implementation detail, tomorrow it could theoretically switch to http.rb. So, I don't see a place where we can warn about this.

[jrochkind]

I was still considering if the actual gemspec requirement would still be good (although I was going to keep it to myself), but @casperisfine's idea seems pretty good to me. To be extra safe (to say hypothetical future AWS gems that don't depend on Net::Protocol at all so it's not in dependencies anymore) if defined?(Net::Protocol::VERSION) && Net::Protocol::VERSION == "0.2.0"

Although just a log warning is easy to miss, at least it's something. (I forget if stderr will go to console in eg Rails rspec). I think I would have deployed the corrupting code to production if I hadn't happened to notice the issues filed here.