Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate the cached value before writing it in the local cache #37587

Merged
merged 1 commit into from Oct 29, 2019

Conversation

@casperisfine
Copy link

casperisfine commented Oct 29, 2019

Discovered by @eljojo.

I think the test case showcase the bug somewhat properly. In short it was already protecting itself from mutation of returned values, but not from mutation of the original value, e.g.:

my_string = "foo"
cache.write('key', my_string)
my_string << "bar"
cache.read('key') # => "foobar"

@rafaelfranca @camilo @etiennebarrie @Edouard-chin

@rails-bot rails-bot bot added the activesupport label Oct 29, 2019
@casperisfine casperisfine force-pushed the Shopify:fix-local-cache-leak branch from e3294bb to 9b09187 Oct 29, 2019
def write_entry(key, value, **options)
@data[key] = value
def write_entry(key, entry, **options)
entry.dup_value!

This comment has been minimized.

Copy link
@eljojo

eljojo Oct 29, 2019

will this do deep duping?

This comment has been minimized.

Copy link
@Edouard-chin

Edouard-chin Oct 29, 2019

Contributor

It does a Marshal.load

@value = Marshal.load(Marshal.dump(@value))

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Oct 29, 2019

I'm pretty sure we have a PR for this already.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Oct 29, 2019

Ah, it was #36656

@rafaelfranca rafaelfranca merged commit 58fe42e into rails:master Oct 29, 2019
2 checks passed
2 checks passed
buildkite/rails Build #64613 passed (14 minutes, 30 seconds)
Details
codeclimate All good!
Details
@Edouard-chin

This comment has been minimized.

Copy link
Contributor

Edouard-chin commented Oct 29, 2019

Yeah #36656 fixed a similar issue but only protecting when returning a value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.