rework errors slightly. Add buffer overflow check for manifest, so we don't create a memory monster by accident

5 files changed, 23 insertions, 10 deletions

diff --git a/ext/phar/phar.c b/ext/phar/phar.c
index 04a0475e68..0ba5978d95 100644
--- a/ext/phar/phar.c
+++ b/ext/phar/phar.c
@@ -163,17 +163,18 @@ PHP_METHOD(PHP_Archive, mapPhar)
 		php_stream_close(fp);\
 		php_error_docref(NULL TSRMLS_CC, E_ERROR, msg, fname);\
 		return;
-#define MAPPHAR_FAIL(msg) efree(buffer);\
+#define MAPPHAR_FAIL(msg) efree(savebuf);\
 		MAPPHAR_ALLOC_FAIL(msg)
 
 	// check for ?>\n and increment accordingly
 	if (-1 == php_stream_seek(fp, halt_offset, SEEK_SET)) {
-		MAPPHAR_FAIL("cannot seek to __HALT_COMPILER(); location in phar \"%s\"")
+		MAPPHAR_ALLOC_FAIL("cannot seek to __HALT_COMPILER(); location in phar \"%s\"")
 	}
 
 	if (FALSE == (buffer = (char *) emalloc(4))) {
 		MAPPHAR_ALLOC_FAIL("memory allocation failed in phar \"%s\"")
 	}
+	savebuf = buffer;
 	if (3 != php_stream_read(fp, buffer, 3)) {
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest)")
 	}
@@ -202,9 +203,9 @@ PHP_METHOD(PHP_Archive, mapPhar)
 
 	i = 0;
 #define PHAR_GET_VAL(var)			\
-	if (buffer > endbuffer) {\
+	if (buffer > endbuffer) {		\
 		MAPPHAR_FAIL("internal corruption of phar \"%s\" (truncated manifest)")\
-	}\
+	}					\
 	unpack_var = (char *) &var;		\
 	var = 0;				\
 	for (i = 0; i < 4; i++) {		\
@@ -217,8 +218,12 @@ PHP_METHOD(PHP_Archive, mapPhar)
 	endbuffer = buffer;
 	PHAR_GET_VAL(manifest_len)
 	buffer -= 4;
+	if (manifest_len > 1048576) {
+		/* prevent serious memory issues by limiting manifest to at most 1 MB in length */
+		MAPPHAR_FAIL("manifest cannot be larger than 1 MB in phar \"%s\"")
+	}
 	if (FALSE == (buffer = (char *) erealloc(buffer, manifest_len))) {
-		MAPPHAR_ALLOC_FAIL("memory allocation failed in phar \"%s\"")
+		MAPPHAR_FAIL("memory allocation failed in phar \"%s\"")
 	}
 	savebuf = buffer;
 	// set the test pointer
diff --git a/ext/phar/tests/002.phpt b/ext/phar/tests/002.phpt
index 8220fae1bb..db5d42f28d 100644
--- a/ext/phar/tests/002.phpt
+++ b/ext/phar/tests/002.phpt
@@ -10,8 +10,7 @@ PHP_Archive::mapPhar(5, 5);
 PHP_Archive::mapPhar(5, 'hio');
 PHP_Archive::mapPhar(5, 'hio', 'hi');
 PHP_Archive::mapPhar(5, 'hio', true, 5, 5);
-__HALT_COMPILER();
-?>
+__HALT_COMPILER(); ?>
 --EXPECTF--
 Warning: PHP_Archive::mapPhar() expects at least 3 parameters, 0 given in %s on line %d
 
diff --git a/ext/phar/tests/005.phpt b/ext/phar/tests/005.phpt
index 2ec6c315a4..62f761f986 100644
--- a/ext/phar/tests/005.phpt
+++ b/ext/phar/tests/005.phpt
@@ -5,7 +5,6 @@ PHP_Archive::mapPhar improper parameters
 --FILE--
 <?php
 PHP_Archive::mapPhar(5, 'hio', false);
-__HALT_COMPILER();
-?>
+__HALT_COMPILER(); ?>
 --EXPECTF--
 Fatal error: PHP_Archive::mapPhar(): internal corruption of phar "%s" (truncated manifest) in %s on line %d
\ No newline at end of file
diff --git a/ext/phar/tests/006.phpt b/ext/phar/tests/006.phpt
index 07992a4b70..d66f2efede 100644
--- a/ext/phar/tests/006.phpt
+++ b/ext/phar/tests/006.phpt
@@ -5,6 +5,6 @@ PHP_Archive::mapPhar improper parameters
 --FILE--
 <?php
 PHP_Archive::mapPhar(5, 'hio', false);
-__HALT_COMPILER(); ?>()a
+__HALT_COMPILER(); ?>()
 --EXPECTF--
 Fatal error: PHP_Archive::mapPhar(): internal corruption of phar "%s" (truncated manifest) in %s on line %d
\ No newline at end of file
diff --git a/ext/phar/tests/007.phpt b/ext/phar/tests/007.phpt
new file mode 100644
index 0000000000..4772364d81
--- /dev/null
+++ b/ext/phar/tests/007.phpt
@@ -0,0 +1,10 @@
+--TEST--
+PHP_Archive::mapPhar improper parameters
+--SKIPIF--
+<?php if (!extension_loaded("phar")) print "skip";?>
+--FILE--
+<?php
+PHP_Archive::mapPhar(5, 'hio', false);
+__HALT_COMPILER(); ?>~~~~
+--EXPECTF--
+Fatal error: PHP_Archive::mapPhar(): manifest cannot be larger than 1 MB in phar "%s" in %s on line %d
\ No newline at end of file