Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address more Clang warnings #17506

Merged
merged 5 commits into from
Jan 21, 2025
+20 −20
Merged

Address more Clang warnings #17506

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d00dd60
Address more Clang warnings
cmb69 Jan 18, 2025
3a11e29
fix
cmb69 Jan 18, 2025
d702114
break instead of duplicating zend_string_copy() call
cmb69 Jan 19, 2025
d4e60f6
Drop doubtful fixes
cmb69 Jan 19, 2025
ccc1cd4
Port https://github.com/libgd/libgd/commit/f1480ab14bd5e2e4b4d83d8f3e…
cmb69 Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ext/com_dotnet/com_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ static zend_function *com_method_get(zend_object **object_ptr, zend_string *name
ITypeComp_Release(bindptr.lptcomp);
break;

case DESCKIND_NONE:
default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps EMPTY_SWITCH_DEFAULT_CASE() instead?

Copy link
Member

@nielsdos nielsdos Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That macro is a misnomer and dangerous, it asserts the path is unreachable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it asserts the path is unreachable

Yes and this seems to be the desired behavior here. The documentation indicates that only the 4 DESCKIND_* cases that were previously handled may be returned. I'd rather crash if a new case appears instead of silently doing the wrong thing with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, you won't necessarily crash. Anything can happen because taking an unreachable path is UB. This can either result in taking the wrong case or doing something completely absurd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that assertions are turned into assumes in release builds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine with an assert(), but __assume() is dangerous, it's comparable to UB, if applied at the wrong place, and I wouldn't fully trust that documentation. Maybe in this case it is better to explicitly list the alternatives, and add a ZEND_ASSERT(0) there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZEND_ASSERT(0) essentially would do the same thing as marking it unreachable. I'd be okay with assert(0) but I predict someone's gonna turn that into ZEND_ASSERT on refactor in the future. ZEND_ASSERT is turned into an assume, and with 0 as an argument it's an assume on something that's false, the compiler can do anything. Can we please not do dangerous programming?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh! Of course, you're right.

Maybe we should actually change ZEND_ASSERT() to not be ZEND_ASSUME() in non-debug builds, and use ZEND_ASSUME() directly, where we are absolutely sure that the code is unreachable (in which case we can likely drop such code, and maybe address compiler warnings by other means). Or something like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know the difference between assume and assert, for me the assertions are there to ensure in debug builds that the preconditions, or the current state is what you expect it to be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transforming assertions into assumes was done on purpose during the phpng development: e1ab2cf
Changing that would have a performance impact as people specifically use it in that way (debug time assertions, production time optimizations)

break;
}
if (TI) {
Expand Down
2 changes: 1 addition & 1 deletion ext/com_dotnet/com_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ static void generate_dispids(php_dispatchex *disp)
zend_string *name = NULL;
zval *tmp, tmp2;
int keytype;
zend_long pid;
zend_ulong pid;
cmb69 marked this conversation as resolved.
Show resolved Hide resolved

if (disp->dispid_to_name == NULL) {
ALLOC_HASHTABLE(disp->dispid_to_name);
Expand Down
2 changes: 2 additions & 0 deletions ext/ffi/ffi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,8 @@ static zend_always_inline zend_string *zend_ffi_mangled_func_name(zend_string *n
case FFI_VECTORCALL_PARTIAL:
return strpprintf(0, "%s@@%zu", ZSTR_VAL(name), zend_ffi_arg_size(type));
# endif
default:
return zend_string_copy(name);
cmb69 marked this conversation as resolved.
Show resolved Hide resolved
}
#endif
return zend_string_copy(name);
Expand Down
2 changes: 1 addition & 1 deletion ext/gd/libgd/gd_interpolation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
#include "gdhelpers.h"
#include "gd_intern.h"

#ifdef _MSC_VER
#if defined(_MSC_VER) && !defined(__clang__)
cmb69 marked this conversation as resolved.
Show resolved Hide resolved
# pragma optimize("t", on)
# include <emmintrin.h>
#endif
Expand Down
4 changes: 2 additions & 2 deletions ext/libxml/libxml.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,9 +433,9 @@ static void *php_libxml_streams_IO_open_wrapper(const char *filename, const char

if (strncasecmp(resolved_path, "file:/", pre_len) == 0
&& '/' != resolved_path[pre_len]) {
xmlChar *tmp = xmlStrdup(resolved_path + pre_len);
xmlChar *tmp = xmlStrdup(BAD_CAST (resolved_path + pre_len));
xmlFree(resolved_path);
resolved_path = tmp;
resolved_path = (char *) tmp;
}
}
#endif
Expand Down
10 changes: 5 additions & 5 deletions ext/mysqlnd/mysqlnd_auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,7 +746,7 @@ static mysqlnd_rsa_t
mysqlnd_sha256_get_rsa_from_pem(const char *buf, size_t len)
{
BCRYPT_KEY_HANDLE ret = 0;
LPSTR der_buf = NULL;
BYTE *der_buf = NULL;
DWORD der_len;
CERT_PUBLIC_KEY_INFO *key_info = NULL;
DWORD key_info_len;
Expand Down Expand Up @@ -789,7 +789,7 @@ mysqlnd_sha256_public_encrypt(MYSQLND_CONN_DATA * conn, mysqlnd_rsa_t server_pub

ZeroMemory(&padding_info, sizeof padding_info);
padding_info.pszAlgId = BCRYPT_SHA1_ALGORITHM;
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, xor_str, passwd_len + 1, &padding_info,
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, (zend_uchar *) xor_str, passwd_len + 1, &padding_info,
NULL, 0, NULL, 0, &server_public_key_len, BCRYPT_PAD_OAEP)) {
DBG_RETURN(0);
}
Expand All @@ -809,7 +809,7 @@ mysqlnd_sha256_public_encrypt(MYSQLND_CONN_DATA * conn, mysqlnd_rsa_t server_pub

*auth_data_len = server_public_key_len;
ret = malloc(*auth_data_len);
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, xor_str, passwd_len + 1, &padding_info,
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, (zend_uchar *) xor_str, passwd_len + 1, &padding_info,
NULL, 0, ret, server_public_key_len, &server_public_key_len, BCRYPT_PAD_OAEP)) {
BCryptDestroyKey((BCRYPT_KEY_HANDLE) server_public_key);
DBG_RETURN(0);
Expand Down Expand Up @@ -1052,7 +1052,7 @@ mysqlnd_caching_sha2_public_encrypt(MYSQLND_CONN_DATA * conn, mysqlnd_rsa_t serv

ZeroMemory(&padding_info, sizeof padding_info);
padding_info.pszAlgId = BCRYPT_SHA1_ALGORITHM;
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, xor_str, passwd_len + 1, &padding_info,
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, (zend_uchar *) xor_str, passwd_len + 1, &padding_info,
NULL, 0, NULL, 0, &server_public_key_len, BCRYPT_PAD_OAEP)) {
DBG_RETURN(0);
}
Expand All @@ -1071,7 +1071,7 @@ mysqlnd_caching_sha2_public_encrypt(MYSQLND_CONN_DATA * conn, mysqlnd_rsa_t serv
}

*crypted = emalloc(server_public_key_len);
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, xor_str, passwd_len + 1, &padding_info,
if (BCryptEncrypt((BCRYPT_KEY_HANDLE) server_public_key, (zend_uchar *) xor_str, passwd_len + 1, &padding_info,
NULL, 0, *crypted, server_public_key_len, &server_public_key_len, BCRYPT_PAD_OAEP)) {
BCryptDestroyKey((BCRYPT_KEY_HANDLE) server_public_key);
DBG_RETURN(0);
Expand Down
2 changes: 1 addition & 1 deletion ext/mysqlnd/mysqlnd_connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,7 +2241,7 @@ mysqlnd_poll(MYSQLND **r_array, MYSQLND **e_array, MYSQLND ***dont_poll, long se
retval = php_select(max_fd + 1, &rfds, &wfds, &efds, tv_p);

if (retval == -1) {
php_error_docref(NULL, E_WARNING, "Unable to select [%d]: %s (max_fd=%d)",
php_error_docref(NULL, E_WARNING, "Unable to select [%d]: %s (max_fd=" PHP_SOCKET_FMT ")",
errno, strerror(errno), max_fd);
DBG_RETURN(FAIL);
}
Expand Down
2 changes: 1 addition & 1 deletion ext/openssl/xp_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ static int php_openssl_win_cert_verify_callback(X509_STORE_CTX *x509_store_ctx,
err_code = e;
}

php_error_docref(NULL, E_WARNING, "Error encoding X509 certificate: %d: %s", err_code, ERR_error_string(err_code, err_buf));
php_error_docref(NULL, E_WARNING, "Error encoding X509 certificate: %lu: %s", err_code, ERR_error_string(err_code, err_buf));
RETURN_CERT_VERIFY_FAILURE(SSL_R_CERTIFICATE_VERIFY_FAILED);
}

Expand Down
2 changes: 1 addition & 1 deletion ext/standard/streamsfuncs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,7 +859,7 @@ PHP_FUNCTION(stream_select)
retval = php_select(max_fd+1, &rfds, &wfds, &efds, tv_p);

if (retval == -1) {
php_error_docref(NULL, E_WARNING, "Unable to select [%d]: %s (max_fd=%d)",
php_error_docref(NULL, E_WARNING, "Unable to select [%d]: %s (max_fd=" PHP_SOCKET_FMT ")",
errno, strerror(errno), max_fd);
RETURN_FALSE;
}
Expand Down
2 changes: 2 additions & 0 deletions main/php_network.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,10 @@ END_EXTERN_C()

#ifdef PHP_WIN32
typedef SOCKET php_socket_t;
#define PHP_SOCKET_FMT "%" PRIxPTR
#else
typedef int php_socket_t;
#define PHP_SOCKET_FMT "%d"
#endif

#ifdef PHP_WIN32
Expand Down
2 changes: 1 addition & 1 deletion main/streams/plain_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ static int php_stdiop_cast(php_stream *stream, int castas, void **ret)
}

*(FILE**)ret = data->file;
data->fd = SOCK_ERR;
data->fd = (int) SOCK_ERR;
bukka marked this conversation as resolved.
Show resolved Hide resolved
}
return SUCCESS;

Expand Down
4 changes: 2 additions & 2 deletions win32/registry.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ static int LoadDirectory(HashTable *directories, HKEY key, char *path, int path_
memset(name, '\0', max_name+1);
memset(value, '\0', max_value+1);

if (RegEnumValue(key, i, name, &name_len, NULL, &type, value, &value_len) == ERROR_SUCCESS) {
if (RegEnumValue(key, i, name, &name_len, NULL, &type, (LPBYTE) value, &value_len) == ERROR_SUCCESS) {
if ((type == REG_SZ) || (type == REG_EXPAND_SZ)) {
zval data;

Expand Down Expand Up @@ -287,7 +287,7 @@ char *GetIniPathFromRegistry()
if (OpenPhpRegistryKey(NULL, &hKey)) {
DWORD buflen = MAXPATHLEN;
reg_location = emalloc(MAXPATHLEN+1);
if(RegQueryValueEx(hKey, PHPRC_REGISTRY_NAME, 0, NULL, reg_location, &buflen) != ERROR_SUCCESS) {
if(RegQueryValueEx(hKey, PHPRC_REGISTRY_NAME, 0, NULL, (LPBYTE) reg_location, &buflen) != ERROR_SUCCESS) {
RegCloseKey(hKey);
efree(reg_location);
reg_location = NULL;
Expand Down
4 changes: 2 additions & 2 deletions win32/wsyslog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void syslog(int priority, const char *message, ...)

void vsyslog(int priority, const char *message, va_list args)
{
LPTSTR strs[2];
LPCSTR strs[2];
unsigned short etype;
char *tmp = NULL;
DWORD evid;
Expand Down Expand Up @@ -120,7 +120,7 @@ void vsyslog(int priority, const char *message, va_list args)

/* report the event */
if (strsw[0] && strsw[1]) {
ReportEventW(PW32G(log_source), etype, (unsigned short) priority, evid, NULL, 2, 0, strsw, NULL);
ReportEventW(PW32G(log_source), etype, (unsigned short) priority, evid, NULL, 2, 0, (LPCWSTR *) strsw, NULL);
free(strsw[0]);
free(strsw[1]);
efree(tmp);
Expand Down
Loading