[intl] Record the ICU error code on Spoofchecker failures

[iliaal]

Spoofchecker::isSuspicious(), ::areConfusable(), ::areBidiConfusable(), ::setChecks(), ::setAllowedLocales() and ::setAllowedChars() warn on U_FAILURE but never record the code, so intl_get_error_code() still reads U_ZERO_ERROR after a failed call. They now record it, which is what SPOOFCHECKER_CHECK_STATUS in spoofchecker_class.h already prescribes for this class.

setChecks() is the only one of the six with a failure a test can reach, since uspoof_setChecks() rejects check bits outside USPOOF_ALL_CHECKS|USPOOF_AUX_INFO. The check methods are still untestable: ICU substitutes malformed UTF-8 rather than failing, and the only route left needs a string over INT32_MAX.

[devnexen]

--- a/ext/intl/spoofchecker/spoofchecker_main.cpp
+++ b/ext/intl/spoofchecker/spoofchecker_main.cpp
@@ -46,6 +46,7 @@
      ret = intl_icu_compat_uspoof_check_utf8(co->uspoof, ZSTR_VAL(text), ZSTR_LEN(text), co->uspoofres, SPOOFCHECKER_ERROR_CODE_P(co));

      if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+             intl_error_set_code(NULL, SPOOFCHECKER_ERROR_CODE(co));
              php_error_docref(NULL, E_WARNING, "(%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));

              if (intl_icu_compat_uspoof_check_result_mismatch(co->uspoofres, ret, &errmask, SPOOFCHECKER_ERROR_CODE_P(co))) {
@@ -83,6 +84,7 @@
              ret = uspoof_areConfusableUTF8(co->uspoof, ZSTR_VAL(s1), (int32_t)ZSTR_LEN(s1), ZSTR_VAL(s2), (int32_t)ZSTR_LEN(s2), SPOOFCHECKER_ERROR_CODE_P(co));
      }
      if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+             intl_error_set_code(NULL, SPOOFCHECKER_ERROR_CODE(co));
              php_error_docref(NULL, E_WARNING, "(%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));
              RETURN_TRUE;
      }
@@ -355,6 +357,7 @@
              ret = uspoof_areBidiConfusableUTF8(co->uspoof, (UBiDiDirection)direction, ZSTR_VAL(s1), (int32_t)ZSTR_LEN(s1), ZSTR_VAL(s2), (int32_t)ZSTR_LEN(s2), SPOOFCHECKER_ERROR_CODE_P(co));
      }
      if (U_FAILURE(SPOOFCHECKER_ERROR_CODE(co))) {
+             intl_error_set_code(NULL, SPOOFCHECKER_ERROR_CODE(co));
              php_error_docref(NULL, E_WARNING, "(%d) %s", SPOOFCHECKER_ERROR_CODE(co), u_errorName(SPOOFCHECKER_ERROR_CODE(co)));
              RETURN_TRUE;
      }

also I do not see the reasoning behing the RETURN_TRUE->RETURN_FALSE changes ?

[iliaal]

Your first hunk is already in the patch, so the only difference is RETURN_FALSE. true from isSuspicious() means suspicious, and the U_FAILURE branch returns before $errorCode is assigned, so the caller cannot separate a detection from an ICU failure. SPOOFCHECKER_CHECK_STATUS in spoofchecker_class.h already spells out that contract, it just has no call sites.

Counter is that true is fail-closed for if ($sc->isSuspicious($name)) { reject(); }. Your call, I'll keep only the error-code recording if you prefer that.

[devnexen]

Yes the error recording change alone is enough.

[iliaal]

Done.

[devnexen]

after looking, it seems setChecks()/setAllowedLocales() could be fixed too ?

[iliaal]

Added, plus setAllowedChars() which had the same gap. setChecks(1 << 20) turned out to be reachable, so there's a test and a NEWS entry now; setAllowedLocales() I still could not make fail from userland.