Merge pull request #3245 from kientzle/kientzle-archive-read-failed-header-recovery

[Read core] Allow format handlers to FAIL a header read

(cherry picked from commit f2f3eb9f50)
This commit is contained in:
Tim Kientzle authored and Martin Matuska committed 2026-07-26 20:17:47 +02:00
1 parent 3de8f12efd
commit 5e09594094
9 files changed
+408 -3

No files matched your search

+4
View File
@@ -538,6 +538,7 @@ libarchive_test_SOURCES= \
libarchive/test/test_read_format_lha_filename.c \
libarchive/test/test_read_format_lha_filename_utf16.c \
libarchive/test/test_read_format_lha_oversize_header.c \
libarchive/test/test_read_format_lha_symlink_missing_target.c \
libarchive/test/test_read_format_mtree.c \
libarchive/test/test_read_format_mtree_crash747.c \
libarchive/test/test_read_format_pax_bz2.c \
@@ -597,6 +598,7 @@ libarchive_test_SOURCES= \
libarchive/test/test_read_format_zip_padded.c \
libarchive/test/test_read_format_zip_sfx.c \
libarchive/test/test_read_format_zip_size_exceeds_declared.c \
libarchive/test/test_read_format_zip_symlink_unsupported_compression.c \
libarchive/test/test_read_format_zip_traditional_encryption_data.c \
libarchive/test/test_read_format_zip_uncompressed_size_off_by_4gib.c \
libarchive/test/test_read_format_zip_winzip_aes.c \
@@ -968,6 +970,7 @@ libarchive_test_EXTRA_DIST=\
libarchive/test/test_read_format_lha_lh6.lzh.uu \
libarchive/test/test_read_format_lha_lh7.lzh.uu \
libarchive/test/test_read_format_lha_oversize_header.lzh.uu \
libarchive/test/test_read_format_lha_symlink_missing_target.lzh.uu \
libarchive/test/test_read_format_lha_withjunk.lzh.uu \
libarchive/test/test_read_format_mtree.mtree.uu \
libarchive/test/test_read_format_mtree_nomagic.mtree.uu \
@@ -1136,6 +1139,7 @@ libarchive_test_EXTRA_DIST=\
libarchive/test/test_read_format_zip_size_exceeds_declared_deflate.zip.uu \
libarchive/test/test_read_format_zip_size_exceeds_declared_stored.zip.uu \
libarchive/test/test_read_format_zip_symlink.zip.uu \
libarchive/test/test_read_format_zip_symlink_unsupported_compression.zip.uu \
libarchive/test/test_read_format_zip_traditional_encryption_data.zip.uu \
libarchive/test/test_read_format_zip_uncompressed_size_off_by_4gib.zip.uu \
libarchive/test/test_read_format_zip_ux.zip.uu \
+1
View File
@@ -85,6 +85,7 @@ state_name(unsigned s)
case ARCHIVE_STATE_NEW: return ("new");
case ARCHIVE_STATE_HEADER: return ("header");
case ARCHIVE_STATE_DATA: return ("data");
case ARCHIVE_STATE_DATA_RECOVERY: return ("data_recovery");
case ARCHIVE_STATE_EOF: return ("eof");
case ARCHIVE_STATE_CLOSED: return ("closed");
case ARCHIVE_STATE_FATAL: return ("fatal");
+26
View File
@@ -61,12 +61,38 @@
#define ARCHIVE_READ_DISK_MAGIC (0xbadb0c5U)
#define ARCHIVE_MATCH_MAGIC (0xcad11c9U)
/*
* Having the state be a bitmask makes it easy to check
* for combinations of allowed states. These should
* generally only be used in public API entry points,
* primarily in archive_read.c, archive_write.c, etc.
* Internal callbacks can rely on the core machinery
* to only call them when appropriate.
*
* Generally checked via `__archive_check_magic()`.
*/
/* Newly created archive object, not yet opened */
#define ARCHIVE_STATE_NEW 1U
/* Archive is ready to read a header. */
#define ARCHIVE_STATE_HEADER 2U
/* A header has been read: client can ask for data
* or they can ask for the next header and
* we'll automatically skip the remaining data. */
#define ARCHIVE_STATE_DATA 4U
/* Similar to STATE_DATA but after a FAILED header:
* the client may not read data, but they may ask
* for the next header and we'll recover. */
#define ARCHIVE_STATE_DATA_RECOVERY 8U
/* End-of-archive has been reached. Client can only
* close or free the archive. */
#define ARCHIVE_STATE_EOF 0x10U
/* Archive is closed; client is only allowed to free the archive. */
#define ARCHIVE_STATE_CLOSED 0x20U
/* Archive is in a FATAL error state: a close request
* is permitted but ignored. */
#define ARCHIVE_STATE_FATAL 0x8000U
/* Any valid (non-fatal) state. */
#define ARCHIVE_STATE_ANY (0xFFFFU & ~ARCHIVE_STATE_FATAL)
struct archive_vtable {
+21 -3
View File
@@ -639,7 +639,8 @@ _archive_read_next_header2(struct archive *_a, struct archive_entry *entry)
int r1 = ARCHIVE_OK, r2;
archive_check_magic(_a, ARCHIVE_READ_MAGIC,
ARCHIVE_STATE_HEADER | ARCHIVE_STATE_DATA,
ARCHIVE_STATE_HEADER | ARCHIVE_STATE_DATA |
ARCHIVE_STATE_DATA_RECOVERY,
"archive_read_next_header");
archive_entry_clear(entry);
@@ -648,8 +649,12 @@ _archive_read_next_header2(struct archive *_a, struct archive_entry *entry)
/*
* If client didn't consume entire data, skip any remainder
* (This is especially important for GNU incremental directories.)
* A header that failed to parse (DATA_RECOVERY) still needs the
* same treatment: whatever of its body the format reader left
* unconsumed must be skipped before we can read the next header.
*/
if (a->archive.state == ARCHIVE_STATE_DATA) {
if (a->archive.state == ARCHIVE_STATE_DATA ||
a->archive.state == ARCHIVE_STATE_DATA_RECOVERY) {
r1 = archive_read_data_skip(&a->archive);
if (r1 == ARCHIVE_EOF)
archive_set_error(&a->archive, EIO,
@@ -687,6 +692,18 @@ _archive_read_next_header2(struct archive *_a, struct archive_entry *entry)
case ARCHIVE_FATAL:
a->archive.state = ARCHIVE_STATE_FATAL;
break;
case ARCHIVE_FAILED:
/*
* This entry's header could not be parsed, so its metadata
* cannot be trusted. ARCHIVE_STATE_DATA_RECOVERY still
* permits skipping past it (the format reader is
* responsible for ensuring that's actually possible), but
* blocks archive_read_data() and friends, which all check
* for ARCHIVE_STATE_DATA specifically and would otherwise
* return content for an entry we don't actually understand.
*/
a->archive.state = ARCHIVE_STATE_DATA_RECOVERY;
break;
}
if (r2 == ARCHIVE_OK || r2 == ARCHIVE_WARN)
@@ -953,7 +970,8 @@ archive_read_data_skip(struct archive *_a)
size_t size;
int64_t offset;
archive_check_magic(_a, ARCHIVE_READ_MAGIC, ARCHIVE_STATE_DATA,
archive_check_magic(_a, ARCHIVE_READ_MAGIC,
ARCHIVE_STATE_DATA | ARCHIVE_STATE_DATA_RECOVERY,
"archive_read_data_skip");
if (a->format->read_data_skip != NULL)
+2
View File
@@ -167,6 +167,7 @@ IF(ENABLE_TEST)
test_read_format_lha_filename.c
test_read_format_lha_filename_utf16.c
test_read_format_lha_oversize_header.c
test_read_format_lha_symlink_missing_target.c
test_read_format_mtree.c
test_read_format_mtree_crash747.c
test_read_format_pax_bz2.c
@@ -226,6 +227,7 @@ IF(ENABLE_TEST)
test_read_format_zip_padded.c
test_read_format_zip_sfx.c
test_read_format_zip_size_exceeds_declared.c
test_read_format_zip_symlink_unsupported_compression.c
test_read_format_zip_traditional_encryption_data.c
test_read_format_zip_uncompressed_size_off_by_4gib.c
test_read_format_zip_winzip_aes.c
@@ -0,0 +1,159 @@
/*-
* Copyright (c) 2026 Tim Kientzle
* 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"
#include <fcntl.h>
/*
* A malformed symlink entry (one whose pathname lacks the required '|'
* separator should FAIL the entry but the client should be able to recover
* and read the following entry.
*
* This was an existing behavior of the LHA reader that was incorrectly
* handled by the read core (it got translated into a FATAL archive condition).
* Recent fixes to the read core enabled this to work as intended.
*/
static struct archive *
open_fixture(const char *refname)
{
struct archive *a;
extract_reference_file(refname);
assert((a = archive_read_new()) != NULL);
assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
assertEqualIntA(a, ARCHIVE_OK,
archive_read_open_filename(a, refname, 4096));
return a;
}
static void
test_recovery(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
char buff[32];
failure("LHA's own ARCHIVE_FAILED from read_header must propagate, "
"not be silently swallowed or crash");
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_next_header() must still be able to advance "
"past a failed entry to the next one");
assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
assertEqualString("second", archive_entry_pathname(ae));
assertEqualInt(11, (int)archive_read_data(a, buff, sizeof(buff)));
assertEqualMem(buff, "hello world", 11);
assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header(a, &ae));
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_read_data(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
char buff[32];
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_data() must refuse content from a failed "
"entry, not return real (or garbage) bytes");
assert(archive_read_data(a, buff, sizeof(buff)) < 0);
failure("the archive must stay unusable after that misuse, not "
"silently recover");
assert(archive_read_next_header(a, &ae) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_read_data_block(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
const void *buff;
size_t size;
int64_t offset;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_data_block() must refuse a failed entry");
assert(archive_read_data_block(a, &buff, &size, &offset) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_seek_data(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_seek_data() must refuse a failed entry");
assert(archive_seek_data(a, 0, SEEK_SET) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_read_data_into_fd(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
int fd;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
fd = open("data_into_fd_lha.out", O_WRONLY | O_CREAT | O_TRUNC, 0644);
assert(fd >= 0);
failure("archive_read_data_into_fd() must refuse a failed entry");
assert(archive_read_data_into_fd(a, fd) < 0);
close(fd);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_explicit_skip(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
assertEqualIntA(a, ARCHIVE_OK, archive_read_data_skip(a));
assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
assertEqualString("second", archive_entry_pathname(ae));
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
DEFINE_TEST(test_read_format_lha_symlink_missing_target)
{
const char *refname =
"test_read_format_lha_symlink_missing_target.lzh";
test_recovery(refname);
test_illegal_read_data(refname);
test_illegal_read_data_block(refname);
test_illegal_seek_data(refname);
test_illegal_read_data_into_fd(refname);
test_explicit_skip(refname);
}
@@ -0,0 +1,11 @@
Level-0 LHA archive with two entries: "bad_symlink" is a symlink
(Unix-extended mode field, S_IFLNK) whose pathname has no '|' separator,
so lha_parse_linkname() can't extract a link target and read_header()
returns ARCHIVE_FAILED; "second" is a normal stored file. See
test_read_format_lha_symlink_missing_target.c.
begin 644 test_read_format_lha_symlink_missing_target.lzh
M+>LM;&@P+0``````````````````"V)A9%]S>6UL:6YK``!5``````#_H0``
J```:]BUL:#`M"P````L````````````&<V5C;VYD:&5L;&\@=V]R;&0`
`
end
@@ -0,0 +1,168 @@
/*-
* Copyright (c) 2026 Tim Kientzle
* 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"
#include <fcntl.h>
/*
* A symlink entry with an unrecognized compression format should FAIL
* (that individual entry is unsupported) without preventing the rest of
* the archive from being read.
*
* The Zip reader previously FAILED the entry but the archive read core
* translated that to FATAL, preventing the client from continuing. Updates
* to the read core allow us to finally handle that correctly.
*/
static struct archive *
open_fixture(const char *refname)
{
struct archive *a;
extract_reference_file(refname);
assert((a = archive_read_new()) != NULL);
assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_all(a));
assertEqualIntA(a, ARCHIVE_OK,
archive_read_open_filename(a, refname, 4096));
return a;
}
/* A well-behaved client that never tries to read data from the failed
* entry must be able to recover and read the rest of the archive. */
static void
test_recovery(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
char buff[32];
failure("a format's ARCHIVE_FAILED from read_header must propagate, "
"not be silently swallowed or crash");
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_next_header() must still be able to advance "
"past a failed entry to the next one");
assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
assertEqualString("second", archive_entry_pathname(ae));
assertEqualInt(11, (int)archive_read_data(a, buff, sizeof(buff)));
assertEqualMem(buff, "hello world", 11);
assertEqualIntA(a, ARCHIVE_EOF, archive_read_next_header(a, &ae));
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
/* A client that instead tries to read real content from the failed entry
* must be refused (not handed garbage, not crash) -- and, matching every
* other API-misuse case in libarchive, the archive object is left
* unusable afterward rather than silently continuing. */
static void
test_illegal_read_data(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
char buff[32];
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_data() must refuse content from a failed "
"entry, not return real (or garbage) bytes");
assert(archive_read_data(a, buff, sizeof(buff)) < 0);
failure("the archive must stay unusable after that misuse, not "
"silently recover");
assert(archive_read_next_header(a, &ae) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_read_data_block(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
const void *buff;
size_t size;
int64_t offset;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_read_data_block() must refuse a failed entry");
assert(archive_read_data_block(a, &buff, &size, &offset) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_seek_data(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
failure("archive_seek_data() must refuse a failed entry");
assert(archive_seek_data(a, 0, SEEK_SET) < 0);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
static void
test_illegal_read_data_into_fd(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
int fd;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
fd = open("data_into_fd.out", O_WRONLY | O_CREAT | O_TRUNC, 0644);
assert(fd >= 0);
failure("archive_read_data_into_fd() must refuse a failed entry");
assert(archive_read_data_into_fd(a, fd) < 0);
close(fd);
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
/* Explicitly calling archive_read_data_skip() (rather than relying on the
* implicit skip inside archive_read_next_header()) is a legal way to
* recover, too. */
static void
test_explicit_skip(const char *refname)
{
struct archive *a = open_fixture(refname);
struct archive_entry *ae;
assertEqualIntA(a, ARCHIVE_FAILED, archive_read_next_header(a, &ae));
assertEqualIntA(a, ARCHIVE_OK, archive_read_data_skip(a));
assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
assertEqualString("second", archive_entry_pathname(ae));
assertEqualInt(ARCHIVE_OK, archive_read_free(a));
}
DEFINE_TEST(test_read_format_zip_symlink_unsupported_compression)
{
const char *refname =
"test_read_format_zip_symlink_unsupported_compression.zip";
test_recovery(refname);
test_illegal_read_data(refname);
test_illegal_read_data_block(refname);
test_illegal_seek_data(refname);
test_illegal_read_data_into_fd(refname);
test_explicit_skip(refname);
}
@@ -0,0 +1,16 @@
ZIP archive with two entries: "bad_symlink" is a symlink (Unix mode
S_IFLNK) whose compression method (200) isn't one of the three the ZIP
reader's symlink-target decompression understands (stored/deflate/LZMA
alone), triggering ARCHIVE_FAILED from read_header(); "second" is a
normal stored file. See
test_read_format_zip_symlink_unsupported_compression.c.
begin 644 test_read_format_zip_symlink_unsupported_compression.zip
M4$L#!!0```#(```````^CAE/"0````D````+````8F%D7W-Y;6QI;FMS;VUE
M=VAE<G502P,$%````````````(412@T+````"P````8```!S96-O;F1H96QL
M;R!W;W)L9%!+`0(4`Q0```#(```````^CAE/"0````D````+````````````
M``#_H0````!B861?<WEM;&EN:U!+`0(4`Q0```````````"%$4H-"P````L`
M```&``````````````"D@3(```!S96-O;F102P4&``````(``@!M````80``
#````
`
end