Improve parameter validation

package.xml

@@ -64,7 +64,8 @@ http://pear.php.net/dtd/package-2.0.xsd">
    </dir>
    <dir name="tests">
     <file role="test" name="params.phpt" />
-    <file role="test" name="vectors.phpt" />
+    <file role="test" name="scrypt_errors.phpt" />
+    <file role="test" name="scrypt_vectors.phpt" />
    </dir>
   </dir>
  </contents>

php_scrypt.c

@@ -30,6 +30,7 @@
 
 #include "php.h"
 #include "php_version.h"
+#include "zend_exceptions.h"
 
 #ifdef PHP_WIN32
 #include "zend_config.w32.h"
@@ -47,6 +48,10 @@
 #endif
 #include "math.h"
 
+#if PHP_VERSION_ID < 80000
+#define RETURN_THROWS() do { ZEND_ASSERT(EG(exception)); (void) return_value; return; } while (0)
+#endif
+
 typedef size_t strsize_t;
 
 static const zend_module_dep scrypt_deps[] = {
@@ -120,43 +125,57 @@ PHP_FUNCTION(scrypt)
     int result;
 
     /* Get the parameters for this call */
-    phpN = -1;
-    phpR = -1;
-    phpP = -1;
-    keyLength = 64;
     raw_output = 0;
-    if (zend_parse_parameters(
+    if (zend_parse_parameters_throw(
             ZEND_NUM_ARGS() TSRMLS_CC, "ssllll|b",
             &password, &password_len, &salt, &salt_len,
             &phpN, &phpR, &phpP, &keyLength, &raw_output
         ) == FAILURE)
     {
-        return;
+        RETURN_THROWS();
     }
 
-    /* Clamp & cast them */
+    /* Checks on the parameters */
+
     castError = 0;
-    cryptN = clampAndCast64("N", phpN, &castError, 1);
-    cryptR = clampAndCast32("r", phpR, &castError, 0);
-    cryptP = clampAndCast32("p", phpP, &castError, 0);
+    cryptN = clampAndCast64(3, "N", phpN, 1);
+	if (EG(exception)) {
+        RETURN_THROWS();
+    }
+    cryptR = clampAndCast32(4, "r", phpR, 0);
+	if (EG(exception)) {
+        RETURN_THROWS();
+    }
+    cryptP = clampAndCast32(5, "p", phpP, 0);
+	if (EG(exception)) {
+		RETURN_THROWS();
+	}
 
-    if (keyLength < 16) {
-        keyLength = -1;
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "Key length is too low, must be greater or equal to 16");
-    } else if (keyLength > (powl(2, 32) - 1) * 32) {
-        keyLength = -1;
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "Key length is too high, must be no more than (2^32 - 1) * 32");
+    if (isPowerOfTwo(cryptN) != 0) {
+#if PHP_VERSION_ID >= 80000
+        zend_argument_error(NULL, 3, "must be a power of 2");
+#else
+		zend_throw_error(zend_ce_error, "scrypt(): Argument #3 ($N) must be a power of 2");
+#endif
+        RETURN_THROWS();
     }
 
-    /* Return out if we've encountered a error with the input parameters */
-    if (castError > 0 || keyLength < 0) {
-        RETURN_FALSE;
+    if (keyLength < 16) {
+#if PHP_VERSION_ID >= 80000
+        zend_argument_error(NULL, 6, "must be greater than or equal to 16");
+#else
+		zend_throw_error(zend_ce_error, "scrypt(): Argument #6 ($key_length) must be greater than or equal to 16");
+#endif
+        RETURN_THROWS();
     }
 
-    /* Checks on the parameters */
-    if (isPowerOfTwo(cryptN) != 0) {
-      php_error_docref(NULL TSRMLS_CC, E_ERROR, "N parameter must be a power of 2");
-      RETURN_FALSE;
+    if (keyLength > (powl(2, 32) - 1) * 32) {
+#if PHP_VERSION_ID >= 80000
+        zend_argument_error(NULL, 6, "must be less than or equal to (2^32 - 1) * 32");
+#else
+		zend_throw_error(zend_ce_error, "scrypt(): Argument #6 ($key_length) must be less than or equal to (2^32 - 1) * 32");
+#endif
+        RETURN_THROWS();
     }
 
     /* Allocate the memory for the output of the key */
@@ -175,7 +194,7 @@ PHP_FUNCTION(scrypt)
         RETURN_FALSE;
     }
 
-    if(!raw_output) {
+    if (!raw_output) {
         /* Encode the output in hex */
         hex = (char*) safe_emalloc(2, keyLength, 1);
         php_hash_bin2hex(hex, buf, keyLength);
@@ -213,21 +232,44 @@ PHP_FUNCTION(scrypt_pickparams)
     int rc;
 
     /* Get the parameters for this call */
-    if (zend_parse_parameters(
+    if (zend_parse_parameters_throw(
             ZEND_NUM_ARGS() TSRMLS_CC, "ldd",
             &maxmem, &memfrac, &maxtime
         ) == FAILURE)
     {
-        return;
+        RETURN_THROWS();
     }
 
-    if(maxmem < 0 || memfrac < 0 || maxtime < 0) {
-        RETURN_FALSE;
+    if (maxmem < 0) {
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, 1, "must be greater than or equal to 0");
+#else
+		zend_throw_error(zend_ce_error, "scrypt_pickparams(): Argument #1 ($max_memory) must be greater than or equal to 0");
+#endif
+		RETURN_THROWS();
+    }
+
+    if (memfrac < 0) {
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, 2, "must be greater than or equal to 0");
+#else
+		zend_throw_error(zend_ce_error, "scrypt_pickparams(): Argument #2 ($memory_fraction) must be greater than or equal to 0");
+#endif
+        RETURN_THROWS();
+    }
+
+    if (maxtime < 0) {
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, 3, "must be greater than or equal to 0");
+#else
+		zend_throw_error(zend_ce_error, "scrypt_pickparams(): Argument #3 ($max_time) must be greater than or equal to 0");
+#endif
+        RETURN_THROWS();
     }
 
     rc = pickparams((size_t) maxmem, memfrac, maxtime, &cryptN, &cryptR, &cryptP);
 
-    if(rc != 0) {
+    if (rc != 0) {
         php_error_docref(NULL TSRMLS_CC, E_WARNING, "Could not determine scrypt parameters.");
         RETURN_FALSE;
     }

php_scrypt_utils.c

@@ -26,26 +26,31 @@
 
 #include "php_scrypt_utils.h"
 #include "php_scrypt.h"
+#include "zend_exceptions.h"
 
 /*
  * Casts a long into a uint64_t.
  *
- * Throws a php error if the value is out of bounds
- * and will return 0. The error varaible will be set to 1, otherwise
- * left intact
+ * Throws a php error if the value is out of bounds and will return 0.
  */
 uint64_t
-clampAndCast64(const char *variableName, long value, int *error, long min)
+clampAndCast64(uint32_t argNum, const char *argName, long value, long min)
 {
     TSRMLS_FETCH();
     if (value <= min)
     {
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "%s is too low.", variableName);
-        *error = 1;
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, argNum, "must be greater than %ld", min);
+#else
+		zend_throw_error(zend_ce_error, "scrypt(): Argument #%d ($%s) must be greater than %ld", argNum, argName, min);
+#endif
         return 0;
     } else if (value > UINT64_MAX) {
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "%s is too high.", variableName);
-        *error = 1;
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, argNum, "is too large");
+#else
+        zend_throw_error(zend_ce_error, "scrypt(): Argument #%d ($%s) is too large", argNum, argName);
+#endif
         return 0;
     }
 
@@ -55,23 +60,27 @@ clampAndCast64(const char *variableName, long value, int *error, long min)
 /*
  * Casts a long into a uint32_t.
  *
- * Throws a php error if the value is out of bounds
- * and will return 0. The error varaible will be set to 1, otherwise
- * left intact
+ * Throws an exception if the value is out of bounds and will return -1.
  */
 uint32_t
-clampAndCast32(const char *variableName, long value, int *error, long min)
+clampAndCast32(uint32_t argNum, const char *argName, long value, long min)
 {
     TSRMLS_FETCH();
     if (value <= min)
     {
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "%s is too low.", variableName);
-        *error = 1;
-        return -1;
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, argNum, "must be greater than %ld", min);
+#else
+		zend_throw_error(zend_ce_error, "scrypt(): Argument #%d ($%s) must be greater than %ld", argNum, argName, min);
+#endif
+		return -1;
     } else if (value > UINT32_MAX) {
-        php_error_docref(NULL TSRMLS_CC, E_ERROR, "%s is too high.", variableName);
-        *error = 1;
-        return -1;
+#if PHP_VERSION_ID >= 80000
+		zend_argument_error(NULL, argNum, "is too large");
+#else
+        zend_throw_error(zend_ce_error, "scrypt(): Argument #%d ($%s) is too large", argNum, argName);
+#endif
+		return -1;
     }
 
     return (uint32_t)value;

php_scrypt_utils.h

@@ -44,22 +44,18 @@
 /*
  * Casts a long into a uint64_t.
  *
- * Throws a php error if the value is out of bounds
- * and will return 0. The error varaible will be set to 1, otherwise
- * left intact
+ * Throws a php error if the value is out of bounds and will return 0.
  */
 uint64_t
-clampAndCast64(const char *variableName, long value, int *error, long min);
+clampAndCast64(uint32_t argNum, const char *argName, long value, long min);
 
 /*
  * Casts a long into a uint32_t.
  *
- * Throws a php error if the value is out of bounds
- * and will return 0. The error varaible will be set to 1, otherwise
- * left intact
+ * Throws an exception if the value is out of bounds and will return -1.
  */
 uint32_t
-clampAndCast32(const char *variableName, long value, int *error, long min);
+clampAndCast32(uint32_t argNum, const char *argName, long value, long min);
 
 /*
  * Checks if the givn number is a power of two

tests/scrypt_errors.phpt

@@ -0,0 +1,51 @@
+--TEST--
+Test scrypt() error conditions
+--SKIPIF--
+<?php if (!extension_loaded("scrypt")) print "skip"; ?>
+--FILE--
+<?php
+
+try {
+	scrypt("", "", 1, 1, 1, 64);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+try {
+	scrypt("", "", 15, 1, 1, 64);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+try {
+	scrypt("", "", 16, 0, 1, 64);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+try {
+	scrypt("", "", 16, 1, 0, 16);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+try {
+	scrypt("", "", 16, 1, 1, 15);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+try {
+	scrypt("", "", 16, 1, 1, PHP_INT_MAX);
+} catch (Error $e) {
+	echo $e->getMessage() . "\n";
+}
+
+?>
+--EXPECT--
+scrypt(): Argument #3 ($N) must be greater than 1
+scrypt(): Argument #3 ($N) must be a power of 2
+scrypt(): Argument #4 ($r) must be greater than 0
+scrypt(): Argument #5 ($p) must be greater than 0
+scrypt(): Argument #6 ($key_length) must be greater than or equal to 16
+scrypt(): Argument #6 ($key_length) must be less than or equal to (2^32 - 1) * 32