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

Don't crash when reading/writing Marshal #3713

Merged
merged 2 commits into from May 20, 2015

Conversation

Projects
None yet
4 participants
@fw42
Contributor

fw42 commented May 19, 2015

@parkr @envygeeks, missed a small detail in the other PR. Should have tested this better :-(

@@ -130,7 +130,7 @@ def add_dependency(path, dependency)
#
# Returns nothing.
def write_metadata
File.open(metadata_file, 'w') do |f|
File.open(metadata_file, 'wb') do |f|

This comment has been minimized.

@fw42

fw42 May 19, 2015

Contributor

Marshal is binary data, so this can crash on some contents :-( Sorry I missed that.

@fw42

fw42 May 19, 2015

Contributor

Marshal is binary data, so this can crash on some contents :-( Sorry I missed that.

This comment has been minimized.

@fw42

fw42 May 19, 2015

Contributor

For reference, the error that you would get is:

lib/jekyll/regenerator.rb:138:in `write': "\xD2" from ASCII-8BIT to UTF-8 (Encoding::UndefinedConversionError)
@fw42

fw42 May 19, 2015

Contributor

For reference, the error that you would get is:

lib/jekyll/regenerator.rb:138:in `write': "\xD2" from ASCII-8BIT to UTF-8 (Encoding::UndefinedConversionError)
@@ -164,6 +164,9 @@ def read_metadata
Marshal.load(content)
rescue TypeError
SafeYAML.load(content)
rescue ArgumentError => e

This comment has been minimized.

@fw42

fw42 May 19, 2015

Contributor

This makes it so that we don't crash the whole process and leave the user wondering if we ever fail to read the metadata for some reason (for example because it crashed because of bugs like the above).

@fw42

fw42 May 19, 2015

Contributor

This makes it so that we don't crash the whole process and leave the user wondering if we ever fail to read the metadata for some reason (for example because it crashed because of bugs like the above).

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 19, 2015

Contributor

Do you mind adding regression tests for these please? 🎯

Contributor

envygeeks commented May 19, 2015

Do you mind adding regression tests for these please? 🎯

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 May 19, 2015

Contributor

Added a test for reading corrupted files. Not sure how to test the other thing (but I don't really think a test is necessary for that).

Contributor

fw42 commented May 19, 2015

Added a test for reading corrupted files. Not sure how to test the other thing (but I don't really think a test is necessary for that).

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 19, 2015

Contributor

I'm 👍 on this fix, I didn't get this problem but it's nice that you noticed it before it became a problem.

Contributor

envygeeks commented May 19, 2015

I'm 👍 on this fix, I didn't get this problem but it's nice that you noticed it before it became a problem.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing May 20, 2015

Member

👍 I didn't have any issues with just w, but I guess it might have broken slightly more sensitive systems.

Member

alfredxing commented May 20, 2015

👍 I didn't have any issues with just w, but I guess it might have broken slightly more sensitive systems.

alfredxing added a commit that referenced this pull request May 20, 2015

@alfredxing alfredxing merged commit f6b34f7 into jekyll:master May 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

goodtouch added a commit to goodtouch/jekyll that referenced this pull request May 26, 2015

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.