mutations icon indicating copy to clipboard operation
mutations copied to clipboard

dup default values to prevent leaks across calls

Open rintaun opened this issue 5 years ago • 3 comments

Modifies InputFilter to return a dup of the given default value rather than the value itself. This prevents leaks across multiple calls of a command.

Previously, if the validation or execution logic of a command mutated the default value object, that change was persisted to subsequent calls of the command and could lead to unexpected behavior.

By duping the default value in InputFilter, any command logic that mutates the value is prevented from leaking.

rintaun avatar Nov 12 '19 21:11 rintaun

Bump. Any possibility of getting this merged? The build failure, fwiw, is due to an error in the build, not something caused by this PR.

rintaun avatar Dec 30 '19 16:12 rintaun

I've been bitten by this problem before, so I'm into this idea. A few notes:


Calling dup on true, false, or nil raises a TypeError on Ruby < 2.4:

$ RBENV_VERSION=2.3.8 ruby -e "puts true.dup"
-e:1:in `dup': can't dup TrueClass (TypeError)
	from -e:1:in `<main>'
$ RBENV_VERSION=2.4.9 ruby -e "puts true.dup"
true

This project still supports Ruby 1.9 (!), so we'll have to avoid calling dup on those values.


There will be a (small) performance penalty from copying the default value on every invocation. If we skip the dup call for frozen? values, applications will have a way to avoid the overhead if necessary, and using the frozen string literal magic comment will give the speed boost for free. This behaviour also seems more correct to me, since calling dup on a frozen object returns an unfrozen one, which is no longer the default that the user provided. It would handle the caveat I mentioned above too, since true, false, and nil are all frozen.


Rebasing against master to pick up https://github.com/cypriss/mutations/commit/36018dec5f0230546a97321c2fda5b584aae97eb should fix CI.

eugeneius avatar Feb 18 '20 16:02 eugeneius

@eugeneius thanks for the feedback! I've rebased and updated the PR with your suggestions :)

rintaun avatar Feb 18 '20 16:02 rintaun