archive_read: don't poison archive when seek is unsupported (fixes #3323)
3.8.8 ("make ARCHIVE_FATAL sticky in data-reading entry points",
commit e1f890dc) made archive_seek_data() set
a->archive.state = ARCHIVE_STATE_FATAL whenever the call returned
ARCHIVE_FATAL. That is correct for genuine I/O / parse failures
returned by a format's seek_data() implementation, but it also
poisoned the archive in two cases where seeking is simply
unsupported for the current format:
1. The seek_data == NULL branch in archive_seek_data() itself
(no format_seek_data_block registered: ustar/tar, the
streaming ZIP reader, ...).
2. rar5_seek_data() in archive_read_support_format_rar5.c, which
unconditionally returns ARCHIVE_FATAL because RAR5 is a
streaming unpacker. This was latent before 3.8.8 because
the read core did not make FATAL sticky; the stickiness
change exposed it.
Both are capability gaps, not stream corruption. ARCHIVE_FATAL
must always be sticky (it means the archive is irrecoverably
damaged), so both sites now return ARCHIVE_FAILED with an error
string instead. ARCHIVE_FAILED is recoverable: capability probes
such as
if (archive_seek_data(a, 0, SEEK_CUR) >= 0) ...
learn that seeking is unavailable while the archive remains
usable for subsequent reads. The sticky-on-genuine-FATAL part of
the original 3.8.8 change is preserved for actual ARCHIVE_FATAL
returns from a format's seek_data() implementation.
The seek_data == NULL branch also reports the condition better: a
format that never registers a seek hook is not illegal use of the
library, so the error is now ARCHIVE_ERRNO_MISC / "Cannot seek
data with this format" instead of ARCHIVE_ERRNO_PROGRAMMER /
"Internal error: No format_seek_data_block function registered".
The rar5 change also matches how the RAR4 reader already reports
the same situation (archive_read_support_format_rar.c:1339
returns ARCHIVE_FAILED for compressed RAR files).
This restores 3.8.7 behaviour for clients that probe seekability
before reading. VLC's skins2 stream extractor
(modules/stream_extractor/archive.c, archive_seek_subentry())
probes with archive_seek_data(a, 0, SEEK_CUR); on a non-seekable
format the poisoned state killed all later archive_read_data()
calls, breaking ZIP-packed .vlt skins on distributions shipping
libarchive 3.8.8.
Tests:
- test_archive_seek_data_unsupported: covers the seek_data==NULL
branch using a ustar archive.
- test_read_format_rar5_seek_data_unsupported: covers the
rar5_seek_data() branch using an existing RAR5 fixture, and
verifies the entry content with verify_data() after the probe.
Both fail on master and pass with this change.
This commit is contained in:
1 parent
f5509ae993
commit
79b06edf6e
6 files changed
+133
-8
No files matched your search
@@ -411,6 +411,7 @@ libarchive_test_SOURCES= \
|
||||
libarchive/test/test_archive_read_open2.c \
|
||||
libarchive/test/test_archive_read_set_options.c \
|
||||
libarchive/test/test_archive_read_support.c \
|
||||
libarchive/test/test_archive_seek_data_unsupported.c \
|
||||
libarchive/test/test_archive_set_error.c \
|
||||
libarchive/test/test_archive_string.c \
|
||||
libarchive/test/test_archive_string_conversion.c \
|
||||
|
||||
@@ -1003,11 +1003,9 @@ archive_seek_data(struct archive *_a, int64_t offset, int whence)
|
||||
"archive_seek_data_block");
|
||||
|
||||
if (a->format->seek_data == NULL) {
|
||||
archive_set_error(&a->archive, ARCHIVE_ERRNO_PROGRAMMER,
|
||||
"Internal error: "
|
||||
"No format_seek_data_block function registered");
|
||||
a->archive.state = ARCHIVE_STATE_FATAL;
|
||||
return (ARCHIVE_FATAL);
|
||||
archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC,
|
||||
"Cannot seek data with this format");
|
||||
return (ARCHIVE_FAILED);
|
||||
}
|
||||
|
||||
r = (a->format->seek_data)(a, offset, whence);
|
||||
|
||||
@@ -4396,13 +4396,15 @@ static int rar5_read_data_skip(struct archive_read *a) {
|
||||
static int64_t rar5_seek_data(struct archive_read *a, int64_t offset,
|
||||
int whence)
|
||||
{
|
||||
(void) a;
|
||||
(void) offset;
|
||||
(void) whence;
|
||||
|
||||
/* We're a streaming unpacker, and we don't support seeking. */
|
||||
/* We're a streaming unpacker, and we don't support seeking.
|
||||
* That's a capability gap, not a fatal error. */
|
||||
archive_set_error(&a->archive, ARCHIVE_ERRNO_MISC,
|
||||
"Seeking of RAR5 files is unsupported");
|
||||
|
||||
return ARCHIVE_FATAL;
|
||||
return (ARCHIVE_FAILED);
|
||||
}
|
||||
|
||||
static int rar5_cleanup(struct archive_read *a) {
|
||||
|
||||
@@ -40,6 +40,7 @@ IF(ENABLE_TEST)
|
||||
test_archive_read_open2.c
|
||||
test_archive_read_set_options.c
|
||||
test_archive_read_support.c
|
||||
test_archive_seek_data_unsupported.c
|
||||
test_archive_set_error.c
|
||||
test_archive_string.c
|
||||
test_archive_string_conversion.c
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
/*-
|
||||
* Copyright (c) 2026 tbontb-iaq
|
||||
* All rights reserved.
|
||||
*
|
||||
* Redistribution and use in source and binary forms, with or without
|
||||
* modification, are permitted provided that the following conditions
|
||||
* are met:
|
||||
* 1. Redistributions of source code must retain the above copyright
|
||||
* notice, this list of conditions and the following disclaimer.
|
||||
* 2. Redistributions in binary form must reproduce the above copyright
|
||||
* notice, this list of conditions and the following disclaimer in the
|
||||
* documentation and/or other materials provided with the distribution.
|
||||
*
|
||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS OR
|
||||
* IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
|
||||
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
|
||||
* IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, INDIRECT,
|
||||
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
|
||||
* NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
|
||||
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
|
||||
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
|
||||
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
|
||||
* THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
|
||||
*/
|
||||
#include "test.h"
|
||||
|
||||
/*
|
||||
* archive_seek_data() returns ARCHIVE_FAILED when the active format
|
||||
* does not implement a seek_data hook. Such "no seeking for this
|
||||
* format" is a capability gap, not stream corruption, and the archive
|
||||
* must remain usable for subsequent reads. See #3323.
|
||||
*/
|
||||
|
||||
DEFINE_TEST(test_archive_seek_data_unsupported)
|
||||
{
|
||||
char buff[8192];
|
||||
size_t used = 0;
|
||||
struct archive *a;
|
||||
struct archive_entry *ae;
|
||||
char rbuf[64];
|
||||
la_ssize_t rd;
|
||||
la_int64_t probe;
|
||||
|
||||
/* Build a small ustar archive in memory. ustar has no seek_data. */
|
||||
assert((a = archive_write_new()) != NULL);
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_write_set_format_ustar(a));
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_write_add_filter_none(a));
|
||||
assertEqualIntA(a, ARCHIVE_OK,
|
||||
archive_write_open_memory(a, buff, sizeof(buff), &used));
|
||||
assert((ae = archive_entry_new()) != NULL);
|
||||
archive_entry_copy_pathname(ae, "hello.txt");
|
||||
archive_entry_set_mode(ae, S_IFREG | 0644);
|
||||
archive_entry_set_size(ae, 11);
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_write_header(a, ae));
|
||||
archive_entry_free(ae);
|
||||
assertEqualIntA(a, 11, archive_write_data(a, "hello world", 11));
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_write_close(a));
|
||||
assertEqualInt(ARCHIVE_OK, archive_write_free(a));
|
||||
|
||||
/* Read it back. */
|
||||
assert((a = archive_read_new()) != NULL);
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_tar(a));
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_read_open_memory(a, buff, used));
|
||||
assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
|
||||
assertEqualString("hello.txt", archive_entry_pathname(ae));
|
||||
|
||||
/*
|
||||
* Capability probe. Must report the missing capability with
|
||||
* ARCHIVE_FAILED and set an error string. 3.8.7 and 3.8.8 both
|
||||
* returned ARCHIVE_FATAL here; ARCHIVE_FAILED is the new
|
||||
* contract, because a missing hook is recoverable.
|
||||
*/
|
||||
probe = archive_seek_data(a, 0, SEEK_CUR);
|
||||
failure("archive_seek_data() on a format without a seek_data hook "
|
||||
"must report ARCHIVE_FAILED (return value contract)");
|
||||
assertEqualInt(ARCHIVE_FAILED, probe);
|
||||
failure("archive_seek_data() must set an error string when "
|
||||
"the format has no seek_data hook");
|
||||
assert(archive_error_string(a) != NULL);
|
||||
|
||||
/*
|
||||
* The probe must NOT have poisoned the archive. Subsequent reads
|
||||
* from the current entry must still succeed and return the real
|
||||
* entry content. This is the 3.8.8 regression.
|
||||
*/
|
||||
rd = archive_read_data(a, rbuf, sizeof(rbuf));
|
||||
failure("archive_read_data() after a non-fatal capability probe "
|
||||
"must still return the entry data (regression in 3.8.8, "
|
||||
"see GitHub issue #3323)");
|
||||
assertEqualInt(11, rd);
|
||||
assertEqualMem(rbuf, "hello world", 11);
|
||||
|
||||
assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header(a, &ae));
|
||||
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
|
||||
}
|
||||
@@ -1586,3 +1586,31 @@ DEFINE_TEST(test_read_format_rar5_unpacked_size_exceeds_declared)
|
||||
|
||||
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
|
||||
}
|
||||
|
||||
/*
|
||||
* RAR5 is a streaming unpacker and never implements seeking.
|
||||
* archive_seek_data() on a RAR5 entry must report unsupported via
|
||||
* ARCHIVE_FAILED without poisoning the archive state. See #3323.
|
||||
*/
|
||||
DEFINE_TEST(test_read_format_rar5_seek_data_unsupported)
|
||||
{
|
||||
const int DATA_SIZE = 1200;
|
||||
uint8_t buff[1200];
|
||||
|
||||
PROLOGUE("test_read_format_rar5_compressed.rar");
|
||||
|
||||
assertA(0 == archive_read_next_header(a, &ae));
|
||||
assertEqualString("test.bin", archive_entry_pathname(ae));
|
||||
|
||||
/* Capability probe. */
|
||||
assertEqualIntA(a, ARCHIVE_FAILED,
|
||||
archive_seek_data(a, 0, SEEK_CUR));
|
||||
assertA(archive_error_string(a) != NULL);
|
||||
|
||||
/* Subsequent read must still return the real entry content. */
|
||||
assertA(DATA_SIZE == archive_read_data(a, buff, DATA_SIZE));
|
||||
assertA(ARCHIVE_EOF == archive_read_next_header(a, &ae));
|
||||
assertA(1 == verify_data(buff, 0, DATA_SIZE));
|
||||
|
||||
EPILOGUE();
|
||||
}
|
||||
Reference in new issue
Block a user