Merge pull request #3009 from kientzle/archive-read-sticky-fatal

Minor cleanups from #2298

(cherry picked from commit 73c4f1c03f)

CMakeLists.txt

@@ -376,35 +376,33 @@ MACRO (GENERATE_LIST_H _listfile _cmlist _target __list_sources)
 ENDMACRO (GENERATE_LIST_H)
 
 #
+# Register individual tests with CTest by scanning DEFINE_TEST() declarations.
 #
-#
-MACRO(DISCOVER_TESTS _component __list_sources)
+MACRO (DISCOVER_TESTS _component __list_sources)
   IF(ENABLE_TEST)
     SET(_sources ${ARGV})
     LIST(REMOVE_AT _sources 0)
-    FOREACH(_src ${_sources})
-      IF(_src MATCHES "test_[^/]+\\.c$")
+    FOREACH (_src ${_sources})
+      IF (_src MATCHES "test_[^/]+\\.c$")
         FILE(STRINGS "${_src}" _lines REGEX "^DEFINE_TEST\(.*\)")
-        FOREACH(_line ${_lines})
-	  STRING(REGEX MATCH "DEFINE_TEST\((.*)\)" _full_match ${_line})
-	  # CMake REGEX seems to handle ( and \( rather inconsistently...
-	  # So we go through an extra dance to definitively strip the parens
-	  STRING(REPLACE "(" "" _test_name_with_paren "${CMAKE_MATCH_1}")
-	  STRING(REPLACE ")" "" _test_name "${_test_name_with_paren}")
-	  SET(_full_test_name "${_component}_${_test_name}")
-          MESSAGE("Found test ${_full_test_name} command ${_component}_test")
-	  ADD_TEST(NAME ${_full_test_name}
-	           COMMAND ${_component}_test -vv
-	                             -r ${CMAKE_CURRENT_SOURCE_DIR}
-				     -s
-				     ${_test_name})
-	  SET_TESTS_PROPERTIES(${_full_test_name} PROPERTIES SKIP_RETURN_CODE 2)
-
-        ENDFOREACH()
-      ENDIF()
-    endforeach()
+        FOREACH (_line ${_lines})
+          STRING(REGEX MATCH "DEFINE_TEST\((.*)\)" _full_match ${_line})
+          # CMake REGEX seems to handle ( and \( rather inconsistently...
+          # So we go through an extra dance to definitively strip the parens
+          STRING(REPLACE "(" "" _test_name_with_paren "${CMAKE_MATCH_1}")
+          STRING(REPLACE ")" "" _test_name "${_test_name_with_paren}")
+          SET(_full_test_name "${_component}_${_test_name}")
+          ADD_TEST(NAME ${_full_test_name}
+                   COMMAND ${_component}_test -vv
+                                   -r ${CMAKE_CURRENT_SOURCE_DIR}
+                                   -s
+                                   ${_test_name})
+          SET_TESTS_PROPERTIES(${_full_test_name} PROPERTIES SKIP_RETURN_CODE 2)
+        ENDFOREACH (_line)
+      ENDIF (_src MATCHES "test_[^/]+\\.c$")
+    ENDFOREACH (_src)
   ENDIF(ENABLE_TEST)
-ENDMACRO(DISCOVER_TESTS __sources)
+ENDMACRO (DISCOVER_TESTS __list_sources)
 
 #
 # Generate installation rules for man pages.

Makefile.am

@@ -18,7 +18,9 @@ BUILT_SOURCES= libarchive/test/list.h tar/test/list.h cpio/test/list.h cat/test/
 #
 check_PROGRAMS= libarchive_test $(bsdtar_test_programs) $(bsdcpio_test_programs) $(bsdcat_test_programs) $(bsdunzip_test_programs)
 TESTS= libarchive_test $(bsdtar_test_programs) $(bsdcpio_test_programs) $(bsdcat_test_programs) $(bsdunzip_test_programs)
-TESTS_ENVIRONMENT= $(libarchive_TESTS_ENVIRONMENT) $(bsdtar_TESTS_ENVIRONMENT) $(bsdcpio_TESTS_ENVIRONMENT) $(bsdcat_TESTS_ENVIRONMENT) $(bsdunzip_TESTS_ENVIRONMENT) _VERBOSITY_LEVEL=2
+TESTS_ENVIRONMENT= $(libarchive_TESTS_ENVIRONMENT) $(bsdtar_TESTS_ENVIRONMENT) $(bsdcpio_TESTS_ENVIRONMENT) $(bsdcat_TESTS_ENVIRONMENT) $(bsdunzip_TESTS_ENVIRONMENT)
+# Uncomment the following to enable verbose test output:
+# TESTS_ENVIRONMENT += _VERBOSITY_LEVEL=2
 # Always build and test both bsdtar and bsdcpio as part of 'distcheck'
 DISTCHECK_CONFIGURE_FLAGS = --enable-bsdtar --enable-bsdcpio
 # The next line is commented out by default in shipping libarchive releases.

libarchive/archive_read_support_format_7zip.c

@@ -1196,17 +1196,6 @@ archive_read_format_7zip_read_data_skip(struct archive_read *a)
 	 * compressed data much more quickly.
 	 */
 	bytes_skipped = skip_stream(a, (size_t)zip->entry_bytes_remaining);
-	/*
-	 * TODO: when archive_read_data() fails on an encrypted entry with
-	 * ARCHIVE_FAILED, the implicit skip from the next
-	 * archive_read_next_header() lands here.  Empirically, skip_stream()
-	 * still returns ARCHIVE_FATAL via extract_pack_stream() on its second
-	 * call (after setup_decode_folder() returned ARCHIVE_FAILED on the
-	 * first), so the FAILED-from-data / FATAL-from-skip asymmetry persists.
-	 * Audit extract_pack_stream() / read_stream() to see whether the second
-	 * call should also surface ARCHIVE_FAILED so the caller can move on
-	 * to entries in subsequent folders.
-	 */
 	if (bytes_skipped < 0)
 		return ((int)bytes_skipped);
 	zip->entry_bytes_remaining = 0;

libarchive/test/test_read_format_cab_mszip_oob.c

@@ -70,18 +70,24 @@ DEFINE_TEST(test_read_format_cab_mszip_oob)
 	assertEqualIntA(a, ARCHIVE_OK, archive_read_support_format_cab(a));
 	assertEqualIntA(a, ARCHIVE_OK, archive_read_open_memory(a, b, total));
 
-	/*
-	 * Drive the read loop the same way the PoC does.  The first
-	 * archive_read_data() call is expected to fail (ARCHIVE_FATAL or
-	 * ARCHIVE_FAILED) because the payload is truncated.  The second
-	 * archive_read_next_header() call is where the OOB access occurs in
-	 * unfixed code; it must return an error rather than corrupt memory.
-	 */
 	struct archive_entry *ae;
 	char buf[4096];
-	while (archive_read_next_header(a, &ae) == ARCHIVE_OK)
-		archive_read_data(a, buf, sizeof(buf));
 
+	/* The single CFFILE header is well-formed and should read cleanly. */
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_next_header(a, &ae));
+	assertEqualString("a", archive_entry_pathname(ae));
+
+	/*
+	 * Reading data must fail: cbUncomp (0xFF6F) exceeds the decoder's
+	 * uncompressed buffer (0x8000), so the bounds check returns FATAL
+	 * rather than writing past the buffer end.
+	 */
+	assertEqualInt(ARCHIVE_FATAL, archive_read_data(a, buf, sizeof(buf)));
+
+	/* The archive is in FATAL state and should remain that way. */
+	assertEqualIntA(a, ARCHIVE_FATAL, archive_read_next_header(a, &ae));
+
+	assertEqualIntA(a, ARCHIVE_OK, archive_read_close(a));
 	assertEqualInt(ARCHIVE_OK, archive_read_free(a));
 	free(b);
 }