Skip to content

mysql related test cases are failing on big endian system #5380

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

Closed
wants to merge 3 commits into from
Closed

mysql related test cases are failing on big endian system #5380

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac656e4
mysql related test cases are failing on big endian systems, updated t…
Apr 10, 2020
9aae7de
updated local_infile and allow_local_infile variable to type unsigned…
Apr 14, 2020
fb6c8ba
As per review comments from @nikic updated libmysql specified STMT_AT…
Apr 15, 2020
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
4 changes: 2 additions & 2 deletions ext/mysqli/mysqli_nonapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,8 +328,8 @@ void mysqli_common_connect(INTERNAL_FUNCTION_PARAMETERS, zend_bool is_real_conne
#if !defined(MYSQLI_USE_MYSQLND)
mysql->mysql->reconnect = MyG(reconnect);
#endif

mysql_options(mysql->mysql, MYSQL_OPT_LOCAL_INFILE, (char *)&MyG(allow_local_infile));
unsigned int allow_local_infile = MyG(allow_local_infile);
mysql_options(mysql->mysql, MYSQL_OPT_LOCAL_INFILE, (char *)&allow_local_infile);

end:
if (!mysqli_resource) {
Expand Down
2 changes: 1 addition & 1 deletion ext/mysqlnd/mysqlnd_ps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1796,7 +1796,7 @@ MYSQLND_METHOD(mysqlnd_stmt, attr_set)(MYSQLND_STMT * const s,
break;
}
case STMT_ATTR_CURSOR_TYPE: {
unsigned int ival = *(unsigned int *) value;
zend_ulong ival = *(zend_ulong *) value;
Copy link
Member

Choose a reason for hiding this comment

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

Here again, libmysql specifies STMT_ATTR_CURSOR_TYPE (and also STMT_ATTR_PREFETCH_ROWS below) as unsigned long * (see https://dev.mysql.com/doc/refman/8.0/en/mysql-stmt-attr-set.html). We should use that type here, in the attr_get method directly below, and in

php-src/ext/mysqli/mysqli_api.c

Line 2374 in fd5dc55

PHP_FUNCTION(mysqli_stmt_attr_get)
.

Copy link
Author

Choose a reason for hiding this comment

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

@nikic Kindly correct me if my understanding is wrong.

  1. As per the definition you have referred you want me to change the typecasting of value to *(unsigned long * ) value for both the cases i.e STMT_ATTR_CURSOR_TYPE & STMT_ATTR_PREFETCH_ROWS in both the function i.e attr_set and attr_get.
  2. I need to change the data type of value variable from zend_ulong to unsigned long in PHP_FUNCTION(mysqli_stmt_attr_get) function to and its function return type accordingly.

Updating diff changes below. Do let me know if any change is required. I have build and tested following changes.

index 80c8ca76fa..0bdfc4f48e 100644
--- a/ext/mysqli/mysqli_api.c
+++ b/ext/mysqli/mysqli_api.c
@@ -2375,7 +2375,7 @@ PHP_FUNCTION(mysqli_stmt_attr_get)
 {
        MY_STMT *stmt;
        zval    *mysql_stmt;
-       zend_ulong      value = 0;
+       unsigned long   value = 0;
        zend_long       attr;
        int             rc;

@@ -2392,7 +2392,7 @@ PHP_FUNCTION(mysqli_stmt_attr_get)
        if (attr == STMT_ATTR_UPDATE_MAX_LENGTH)
                value = *((my_bool *)&value);
 #endif
-       RETURN_LONG((zend_ulong)value);
+       RETURN_LONG((unsigned long)value);
 }
 /* }}} */

diff --git a/ext/mysqlnd/mysqlnd_ps.c b/ext/mysqlnd/mysqlnd_ps.c
index 9d56f6e032..01ee53e0a8 100644
--- a/ext/mysqlnd/mysqlnd_ps.c
+++ b/ext/mysqlnd/mysqlnd_ps.c
@@ -1796,8 +1796,8 @@ MYSQLND_METHOD(mysqlnd_stmt, attr_set)(MYSQLND_STMT * const s,
                        break;
                }
                case STMT_ATTR_CURSOR_TYPE: {
-                       zend_ulong ival = *(zend_ulong *) value;
-                       if (ival > (zend_ulong) CURSOR_TYPE_READ_ONLY) {
+                       unsigned long ival = *(unsigned long *) value;
+                       if (ival > (unsigned long) CURSOR_TYPE_READ_ONLY) {
                                SET_CLIENT_ERROR(stmt->error_info, CR_NOT_IMPLEMENTED, UNKNOWN_SQLSTATE, "Not implemented");
                                DBG_INF("FAIL");
                                DBG_RETURN(FAIL);
@@ -1806,7 +1806,7 @@ MYSQLND_METHOD(mysqlnd_stmt, attr_set)(MYSQLND_STMT * const s,
                        break;
                }
                case STMT_ATTR_PREFETCH_ROWS: {
-                       unsigned int ival = *(unsigned int *) value;
+                       unsigned long ival = *(unsigned long *) value;
                        if (ival == 0) {
                                ival = MYSQLND_DEFAULT_PREFETCH_ROWS;
                        } else if (ival > 1) {
@@ -1845,10 +1845,10 @@ MYSQLND_METHOD(mysqlnd_stmt, attr_get)(const MYSQLND_STMT * const s,
                        *(zend_bool *) value= stmt->update_max_length;
                        break;
                case STMT_ATTR_CURSOR_TYPE:
-                       *(zend_ulong *) value= stmt->flags;
+                       *(unsigned long *) value= stmt->flags;
                        break;
                case STMT_ATTR_PREFETCH_ROWS:
-                       *(zend_ulong *) value= stmt->prefetch_rows;
+                       *(unsigned long *) value= stmt->prefetch_rows;
                        break;
                default:
                        DBG_RETURN(FAIL);

Copy link
Member

Choose a reason for hiding this comment

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

Those changes look right to me. However, there is one more place that needs to be adjusted:

php-src/ext/mysqli/mysqli_api.c

Lines 2349 to 2360 in fd5dc55

switch (attr) {
#if MYSQL_VERSION_ID >= 50107
case STMT_ATTR_UPDATE_MAX_LENGTH:
mode_b = (my_bool) mode_in;
mode_p = &mode_b;
break;
#endif
default:
mode = mode_in;
mode_p = &mode;
break;
}

The mode variable used there should also be unsigned long.

Copy link
Author

Choose a reason for hiding this comment

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

@nikic but mode_in variable is of type zend_long used during assignment?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that's fine (can add an explicit cast if you like). The actual values involved here are small, it's just important that the type of the variable whose address we take is correct.

Copy link
Author

Choose a reason for hiding this comment

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

@nikic kindly ignore my above comment, I had not covered the implementation for same. I will update earlier said change and raise new commit after testing.Thanks.

if (ival > (zend_ulong) CURSOR_TYPE_READ_ONLY) {
SET_CLIENT_ERROR(stmt->error_info, CR_NOT_IMPLEMENTED, UNKNOWN_SQLSTATE, "Not implemented");
DBG_INF("FAIL");
Expand Down
4 changes: 2 additions & 2 deletions ext/pdo_mysql/mysql_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
/* handle MySQL options */
if (driver_options) {
zend_long connect_timeout = pdo_attr_lval(driver_options, PDO_ATTR_TIMEOUT, 30);
zend_long local_infile = pdo_attr_lval(driver_options, PDO_MYSQL_ATTR_LOCAL_INFILE, 0);
unsigned int local_infile = (unsigned int) pdo_attr_lval(driver_options, PDO_MYSQL_ATTR_LOCAL_INFILE, 0);
zend_string *init_cmd = NULL;
#ifndef PDO_USE_MYSQLND
zend_string *default_file = NULL, *default_group = NULL;
Expand Down Expand Up @@ -779,7 +779,7 @@ static int pdo_mysql_handle_factory(pdo_dbh_t *dbh, zval *driver_options)
} else {
#if defined(MYSQL_OPT_LOCAL_INFILE) || defined(PDO_USE_MYSQLND)
// in case there are no driver options disable 'local infile' explicitly
zend_long local_infile = 0;
unsigned int local_infile = 0;
if (mysql_options(H->server, MYSQL_OPT_LOCAL_INFILE, (const char *)&local_infile)) {
pdo_mysql_error(dbh);
goto cleanup;
Expand Down