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.

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

Fix wrong value of type YEAR on big endian environment. #921

Conversation

junaruga
Copy link
Contributor

@junaruga junaruga commented Dec 6, 2017

This PR for #916 issue "1.2".

I checked it like this.

diff --git a/ext/mysql2/result.c b/ext/mysql2/result.c
index ccb49a5..b2d4c87 100644
--- a/ext/mysql2/result.c
+++ b/ext/mysql2/result.c
@@ -413,15 +413,25 @@ static VALUE rb_mysql_result_fetch_row_stmt(VALUE self, MYSQL_FIELD * fields, co
           }
           break;
         case MYSQL_TYPE_SHORT:        // short int
+        case MYSQL_TYPE_YEAR:         // short int
+          printf("debug rb_mysql_result_fetch_row_stmt type: [%d]\n", result_buffer->buffer_type);
           if (result_buffer->is_unsigned) {
             val = UINT2NUM(*((unsigned short int*)result_buffer->buffer));
+            printf("debug rb_mysql_result_fetch_row_stmt type: [%d], unsinged. buffer: [%d], length: [%lu], buffer_length: [%lu]\n",
+              result_buffer->buffer_type,
+              *((unsigned short int*)result_buffer->buffer),
+              *((unsigned long*)result_buffer->length), result_buffer->buffer_length);
           } else  {
             val = INT2NUM(*((short int*)result_buffer->buffer));
+            printf("debug rb_mysql_result_fetch_row_stmt type: [%d] singed. [%d], length: [%lu], buffer_length: [%lu]\n",
+              result_buffer->buffer_type,
+              *((short int*)result_buffer->buffer),
+              *((unsigned long*)result_buffer->length), result_buffer->buffer_length);
           }

The result is like this on little endian. (MYSQL_TYPE_YEAR value is 13.)

debug rb_mysql_result_fetch_row_stmt type: [13]
debug rb_mysql_result_fetch_row_stmt type: [13], unsinged. buffer: [2009], length: [2], buffer_length: [4]

The value is 131661824 on big endian environment as you know.

Seeing the source code in mariadb-connector-c v3.0.2 (v3.0.3 is not released yet), the type year looks defined as short int.

https://github.com/MariaDB/mariadb-connector-c/blob/v3.0.2/libmariadb/ma_stmt_codec.c#L443

void ps_fetch_int16(MYSQL_BIND *r_param, const MYSQL_FIELD * const field,
                                unsigned char **row)
...
    case MYSQL_TYPE_YEAR:
    case MYSQL_TYPE_SHORT:
        ps_fetch_from_1_to_8_bytes(r_param, field, row, 2);

https://github.com/MariaDB/mariadb-connector-c/blob/v3.0.2/libmariadb/ma_stmt_codec.c#L254

r_param->is_unsigned ? NUMERIC_TRUNCATION(val, 0, UINT_MAX16) : NUMERIC_TRUNCATION(val, INT_MIN16, INT_MAX16)

https://github.com/MariaDB/mariadb-connector-c/blob/v3.0.2/include/ma_global.h#L549-L551

#define INT_MIN16       (~0x7FFF)
#define INT_MAX16       0x7FFF
#define UINT_MAX16 0xFFFF

unsigned YEAR: between 0 (0x00 0x00) and UINT_MAX16 (0xff 0xff) that is unsigned int short
signed YEAR: (I am not sure signed YEAR is possible): between INT_MIN16 and INT_MAX16: int short

I tested this patch on big endian environment. And that's okay.
But I want to know why it was "int" previously.
The definition of YEAR is different between mariadb-connector-c, MySQL server and Mariadb server?
Do you remember it?

Thank you.

@sodabrew
Copy link
Collaborator

sodabrew commented Dec 6, 2017

According to https://dev.mysql.com/doc/refman/5.7/en/c-api-prepared-statement-data-structures.html YEAR is an unsigned int field in the MYSQL_TIME structure.

According to https://dev.mysql.com/doc/refman/5.7/en/c-api-prepared-statement-type-codes.html it's a short int, but should have type MYSQL_TYPE_SHORT!

@sodabrew
Copy link
Collaborator

sodabrew commented Dec 6, 2017

To make this PR internally consistent, please adjust https://github.com/junaruga/mysql2/blob/8ef0b8fd78b3b95b6e8afb154e931f316c2be40b/ext/mysql2/result.c#L236 as well.

It looks like the right change, but it's not clear to me if this is another client library incompatibility. Let's one of us run down the equivalent code in the MySQL Connector/C to confirm.

@junaruga
Copy link
Contributor Author

junaruga commented Dec 6, 2017

Thanks for the information.

According to https://dev.mysql.com/doc/refman/5.7/en/c-api-prepared-statement-data-structures.html YEAR is an unsigned int field in the MYSQL_TIME structure.

According to https://dev.mysql.com/doc/refman/5.7/en/c-api-prepared-statement-type-codes.html it's a short int, but should have type MYSQL_TYPE_SHORT!

That's interesting.
And the official information for MySQL is what I wanted to know.
Thanks for that.

To make this PR internally consistent, please adjust https://github.com/junaruga/mysql2/blob/8ef0b8fd78b3b95b6e8afb154e931f316c2be40b/ext/mysql2/result.c#L236 as well.

Okay, I will change the part too. And let me run it with mariadb-connector-c on big endian evironment at first.

It looks like the right change, but it's not clear to me if this is another client library incompatibility. Let's one of us run down the equivalent code in the MySQL Connector/C to confirm.

You meant client logic for mysql-server?
https://github.com/mysql/mysql-server/tree/5.7/client

I found the C++ connector on MySQL right now, though I do not know "MySQL Connector/C".
https://github.com/mysql/mysql-connector-cpp

@sodabrew
Copy link
Collaborator

sodabrew commented Dec 6, 2017

The download bundles for the MySQL Connector/C are here: https://dev.mysql.com/downloads/connector/c/

Looking for the source code repo.

@junaruga junaruga force-pushed the hotfix/wrong-type-year-on-big-endian-environment branch from 8ef0b8f to e40e7f7 Compare December 6, 2017 19:31
@junaruga
Copy link
Contributor Author

junaruga commented Dec 6, 2017

Okay, I will change the part too. And let me run it with mariadb-connector-c on big endian evironment at first.

Checked. that's okay!

Let's proceed it.

@sodabrew sodabrew merged commit 51665eb into brianmario:master Dec 6, 2017
@sodabrew sodabrew added this to the 0.5.0 milestone Dec 6, 2017
@sodabrew
Copy link
Collaborator

sodabrew commented Dec 6, 2017

Remind myself to mark this for cherry-pick to 0.4.x as well.

@junaruga
Copy link
Contributor Author

junaruga commented Dec 6, 2017

@junaruga
Copy link
Contributor Author

junaruga commented Dec 6, 2017

Thanks for the merging!

@junaruga junaruga deleted the hotfix/wrong-type-year-on-big-endian-environment branch December 6, 2017 19:50
jeremy added a commit to jeremy/mysql2 that referenced this pull request Dec 11, 2017
* master:
  GitHub is HTTPS by default (brianmario#922)
  Fix wrong value of type YEAR on big endian environment. (brianmario#921)
  Avoid 0 byte allocation call when statement takes no parameters
  Small spec improvement for RSpec style
  Travis CI for Mac OS X builds use the same steps again
  More specific exception classes (brianmario#911)
  Travis CI matrix add Ruby 2.5 and switch Mac OS X to Ruby 2.3 (ala High Sierra)
  README text for query options on Statement#execute
  Accept query options on Statement#execute (brianmario#912)
  Drop 1.9.3 support (brianmario#913)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants