Raise Gem::Package::FormatError when gem encounters corrupt EOF #6882
Conversation
martinemde
changed the title
| @@ -363,7 +365,7 @@ def digest(entry) # :nodoc: | |||
| algorithms.each do |algorithm| | |||
| digester = Gem::Security.create_digest(algorithm) | |||
|
|
|||
| digester << entry.read(16_384) until entry.eof? | |||
| digester << entry.readpartial(16_384) until entry.eof? | |||
There was a problem hiding this comment.
Changed to readpartial to benefit from automatically raising EOFError if the entry is corrupt. Otherwise it tries to shovel nil into the digester.
| check_closed | ||
|
|
||
| return nil if @read >= @header.size | ||
| return nil if eof? |
There was a problem hiding this comment.
eof? performs check_closed. Similar changes below too.
| if ret.nil? | ||
| return maxlen ? nil : "" # IO.read returns nil on EOF with len argument |
There was a problem hiding this comment.
This is the core change. If io.read returns nil, handle it appropriately depending on if maxlen was given.
| size_read = @io.read([pending, 4096].min).size | ||
| raise UnexpectedEOF if @io.eof? | ||
| ret = @io.readpartial([pending, 4096].min) | ||
| size_read = ret.size |
There was a problem hiding this comment.
Same problem here where we ignored the possible nil return from read.
I also removed UnexpectedEOF here. I think there's two problems: If the read above hit an EOF without reading any bytes, then .size would raise. If it properly read to right up to the EOF, like if this was the last file in the archive, then this would raise when it should not. The only reason to raise here is if we hit a normal EOFError due to readpartial, so I just let readpartial handle raising.
We could remove the UnexpectedEOF constant also. It was only (never) used here, so I think it's safe to remove completely. Besides, all EOFErrors are unexpected. 🤷♂️
| @@ -946,6 +946,87 @@ def test_verify_corrupt | |||
| tf.close! | |||
| end | |||
|
|
|||
| def test_verify_corrupt_tar_metadata_entry | |||
| gem = tar_file_header("metadata.gz", "", 0, 999, Time.now) | |||
There was a problem hiding this comment.
verify handles metadata, checksums, and data.tar.gz slightly differently, raising different errors when each is corrupt. That's why there's so many new tests here for seemingly the same thing.
test/rubygems/test_gem_package.rb
Outdated
| package.extract_files @destination | ||
| end | ||
|
|
||
| assert_equal "end of file reached in corrupt.gem", e.message |
There was a problem hiding this comment.
Should I make the error better? Currently it's just a passthru error message for EOFError or Zlib's "not in gzip format" error. We could be more clear about it being "corrupt". Opinions?
|
Looks like the errors are legitimate. I'll work on them today. |
martinemde
force-pushed
the
martinemde/test-corrupt-gem-archives
branch
from
128c759
to
9323ad1
Compare
Gem::Package::TarReader::Entry now raises EOFError or returns nil appropriately based on Ruby core IO.read and IO.readpartial behavior. Zlib will respond accordingly by raising Zlib::GzipFile::Error on EOF. When verifying a gem or extracting contents, raise FormatError similar to other cases of corrupt gems. Addresses a bug where Gem::Package would attempt to call size on nil instead of raising a more descriptive and useful error, leading users to assume the problem is internal to rubygems. Remove unused error class TarReader::UnexpectedEOF that was never raised since the NoMethodError on nil would happen first. Use EOFError instead.
martinemde
force-pushed
the
martinemde/test-corrupt-gem-archives
branch
from
efc7c52
to
0b1d9fc
Compare
What was the end-user or developer problem that led to this PR?
Fixes #5981 which raised a
NoMethodErrorwhen the real problem was attempting to read a corrupted gem file.What is your fix for the problem, implemented in this PR?
Gem::Package::TarReader::EntryraisesEOFErroror returnsnilappropriately based on Ruby coreIO.readandIO.readpartialbehavior.Zlib handles
EOFErrorandnilreads by raisingZlib::GzipFile::Error.When verifying a gem or extracting contents, rescue these expected
EOFErrors and Zlib errors to return appropriate FormatError for a corrupt gem.Started as a fix for a bug where
sizewas called onnilbecauseTarReaderdidn't handle unexpectednilreturned fromIO.read.Make sure the following tasks are checked