ext/shmop: Refactoring + test coverage

ext/shmop/shmop.c

@@ -133,25 +133,41 @@ PHP_MINFO_FUNCTION(shmop)
 /* {{{ gets and attaches a shared memory segment */
 PHP_FUNCTION(shmop_open)
 {
-	zend_long key, mode, size;
+	zend_long key, permissions, size;
 	php_shmop *shmop;
 	struct shmid_ds shm;
 	char *flags;
 	size_t flags_len;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "lsll", &key, &flags, &flags_len, &mode, &size) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "lsll", &key, &flags, &flags_len, &permissions, &size) == FAILURE) {
 		RETURN_THROWS();
 	}
 
 	if (flags_len != 1) {
-		zend_argument_value_error(2, "must be a valid access mode");
+		zend_argument_value_error(2, "must be a valid access mode, which is one of \"a\", \"c\", \"n\", or \"w\"");
 		RETURN_THROWS();
 	}
 
+	// TODO Check that permissions are valid?
+	if (permissions < 0) {
+		zend_argument_value_error(3, "must be greater or equal than 0");
+		RETURN_THROWS();
+	}
+
+	if (size < 0) {
+		zend_argument_value_error(4, "must be greater or equal than 0");
+		RETURN_THROWS();
+	}
+
+	/* There is some implications from the php.net docs that if reopening a segment both permision and size must be 0
+	 * Do something to check this is hold? Or just split the modes into seperate functions
+	 */
+
+
 	object_init_ex(return_value, shmop_ce);
 	shmop = Z_SHMOP_P(return_value);
 	shmop->key = key;
-	shmop->shmflg |= mode;
+	shmop->shmflg |= permissions;
 
 	switch (flags[0])
 	{
@@ -173,7 +189,7 @@ PHP_FUNCTION(shmop_open)
 			*/
 			break;
 		default:
-			zend_argument_value_error(2, "must be a valid access mode");
+			zend_argument_value_error(2, "must be a valid access mode, which is one of \"a\", \"c\", \"n\", or \"w\"");
 			goto err;
 	}
 
@@ -231,11 +247,15 @@ PHP_FUNCTION(shmop_read)
 	shmop = Z_SHMOP_P(shmid);
 
 	if (start < 0 || start > shmop->size) {
-		zend_argument_value_error(2, "must be between 0 and the segment size");
+		zend_argument_value_error(2, "must be between 0 and the segment size of %d", shmop->size);
 		RETURN_THROWS();
 	}
 
-	if (count < 0 || start > (ZEND_LONG_MAX - count) || start + count > shmop->size) {
+	if (count <= 0) {
+		zend_argument_value_error(3, "must be greater than 0");
+		RETURN_THROWS();
+	}
+	if (start > (ZEND_LONG_MAX - count) || start + count > shmop->size) {
 		zend_argument_value_error(3, "is out of range");
 		RETURN_THROWS();
 	}
@@ -280,7 +300,6 @@ PHP_FUNCTION(shmop_size)
 PHP_FUNCTION(shmop_write)
 {
 	php_shmop *shmop;
-	zend_long writesize;
 	zend_long offset;
 	zend_string *data;
 	zval *shmid;
@@ -297,14 +316,19 @@ PHP_FUNCTION(shmop_write)
 	}
 
 	if (offset < 0 || offset > shmop->size) {
-		zend_argument_value_error(3, "is out of range");
+		zend_argument_value_error(3, "must be between 0 and the segment size of %d", shmop->size);
+		RETURN_THROWS();
+	}
+
+	if (ZSTR_LEN(data) > shmop->size - (size_t)offset) {
+		zend_argument_value_error(2, "cannot write data of size %zu from offset " ZEND_LONG_FMT
+			" into a segment of size %d", ZSTR_LEN(data), offset, shmop->size);
 		RETURN_THROWS();
 	}
 
-	writesize = ((zend_long)ZSTR_LEN(data) < shmop->size - offset) ? (zend_long)ZSTR_LEN(data) : shmop->size - offset;
-	memcpy(shmop->addr + offset, ZSTR_VAL(data), writesize);
+	memcpy(shmop->addr + ((size_t)offset), ZSTR_VAL(data), ZSTR_LEN(data));
 
-	RETURN_LONG(writesize);
+	RETURN_LONG(ZSTR_LEN(data));
 }
 /* }}} */
 
@@ -321,7 +345,7 @@ PHP_FUNCTION(shmop_delete)
 	shmop = Z_SHMOP_P(shmid);
 
 	if (shmctl(shmop->shmid, IPC_RMID, NULL)) {
-		php_error_docref(NULL, E_WARNING, "Can't mark segment for deletion (are you the owner?)");
+		php_error_docref(NULL, E_WARNING, "Cannot mark segment for deletion");
 		RETURN_FALSE;
 	}
 

ext/shmop/tests/cannot_construct_class_via_new.phpt

@@ -0,0 +1,14 @@
+--TEST--
+Shmop class must not be instantiatable
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+try {
+    var_dump(new Shmop());
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+?>
+--EXPECT--
+Error: Cannot directly construct Shmop, use shmop_open() instead

ext/shmop/tests/shmop_open_errors.phpt

@@ -0,0 +1,45 @@
+--TEST--
+shmop_open errors
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+$key = 9990;
+try {
+    $shmop = shmop_open($key, '', 0, 0);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_open($key, 'wrong_mode', 0, 0);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_open($key, 'q', 0, 0);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_open($key, 'r', -1, 0);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_open($key, 'r', 0, -1);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_open($key, 'c', 0, 0);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+?>
+--EXPECT--
+ValueError: shmop_open(): Argument #2 ($mode) must be a valid access mode, which is one of "a", "c", "n", or "w"
+ValueError: shmop_open(): Argument #2 ($mode) must be a valid access mode, which is one of "a", "c", "n", or "w"
+ValueError: shmop_open(): Argument #2 ($mode) must be a valid access mode, which is one of "a", "c", "n", or "w"
+ValueError: shmop_open(): Argument #3 ($permissions) must be greater or equal than 0
+ValueError: shmop_open(): Argument #4 ($size) must be greater or equal than 0
+ValueError: shmop_open(): Argument #4 ($size) must be greater than 0 for the "c" and "n" access modes

ext/shmop/tests/shmop_open_n_identical_segment.phpt

@@ -0,0 +1,13 @@
+--TEST--
+Cannot create existing segment with n mode
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+$key = 9980;
+$shm_id = shmop_open($key, 'c', 0o644, 1024);
+$shm_id2 = shmop_open($key, 'n', 0o644, 2048);
+shmop_delete($shm_id);
+?>
+--EXPECTF--
+Warning: shmop_open(): Unable to attach or create shared memory segment "File exists" in %s on line %d

ext/shmop/tests/shmop_open_warnings.phpt

@@ -0,0 +1,26 @@
+--TEST--
+shmop extension error messages
+--CREDITS--
+edgarsandi - <[email protected]>
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+
+// Warning outputs: Unable to attach or create shared memory segment
+var_dump(shmop_open(0, 'a', 0644, 1024));
+
+// Shared memory segment size must be greater than zero
+try {
+    var_dump(shmop_open(0, 'a', 0644, 1024));
+} catch (ValueError $exception) {
+    echo $exception->getMessage() . "\n";
+}
+?>
+--EXPECTF--
+
+Warning: shmop_open(): Unable to attach or create shared memory segment "%s" in %s on line %d
+bool(false)
+
+Warning: shmop_open(): Unable to attach or create shared memory segment "%s" in %s on line %d
+bool(false)

ext/shmop/tests/shmop_read_errors.phpt

@@ -0,0 +1,41 @@
+--TEST--
+shmop_read errors
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+$key = 9991;
+$shm_id = shmop_open($key, 'n', 0o644, 1024);
+try {
+    $shmop = shmop_read($shm_id, -1, 100);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_read($shm_id, 1400, 100);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_read($shm_id, 0, -1);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_read($shm_id, 0, 1400);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    $shmop = shmop_read($shm_id, 1000, 25);
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+shmop_delete($shm_id);
+?>
+--EXPECT--
+ValueError: shmop_read(): Argument #2 ($offset) must be between 0 and the segment size of 1024
+ValueError: shmop_read(): Argument #2 ($offset) must be between 0 and the segment size of 1024
+ValueError: shmop_read(): Argument #3 ($size) must be greater than 0
+ValueError: shmop_read(): Argument #3 ($size) is out of range
+ValueError: shmop_read(): Argument #3 ($size) is out of range

ext/shmop/tests/shmop_write_errors.phpt

@@ -0,0 +1,25 @@
+--TEST--
+shmop_write errors
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+$key = 9992;
+$shm_id = shmop_open($key, 'n', 0o644, 16);
+try {
+    var_dump(shmop_write($shm_id, 'text to try write', -10));
+    var_dump(shmop_read($shm_id, 0, 16));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+try {
+    var_dump(shmop_write($shm_id, 'text to try write', 20));
+    var_dump(shmop_read($shm_id, 0, 16));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+shmop_delete($shm_id);
+?>
+--EXPECT--
+ValueError: shmop_write(): Argument #3 ($offset) must be between 0 and the segment size of 16
+ValueError: shmop_write(): Argument #3 ($offset) must be between 0 and the segment size of 16

ext/shmop/tests/shmop_write_partial.phpt

@@ -0,0 +1,26 @@
+--TEST--
+shmop_write with a partial write
+--EXTENSIONS--
+shmop
+--FILE--
+<?php
+const SEGMENT_SIZE = 16;
+
+$key = 9998;
+$shm_id = shmop_open($key, 'n', 0o644, SEGMENT_SIZE);
+
+var_dump(shmop_write($shm_id, str_repeat('a', SEGMENT_SIZE), /* offset */ 0));
+var_dump(shmop_read($shm_id, 0, SEGMENT_SIZE));
+
+try {
+    var_dump(shmop_write($shm_id, str_repeat('b', 8), /* offset */ SEGMENT_SIZE-5));
+    var_dump(shmop_read($shm_id, 0, SEGMENT_SIZE));
+} catch (Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+shmop_delete($shm_id);
+?>
+--EXPECT--
+int(16)
+string(16) "aaaaaaaaaaaaaaaa"
+ValueError: shmop_write(): Argument #2 ($data) cannot write data of size 8 from offset 11 into a segment of size 16