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 a segmentation fault which Mysql2::Statement#fields causes. #860

Closed
wants to merge 3 commits into from

Conversation

kanata2
Copy link

@kanata2 kanata2 commented Jun 22, 2017

I found a bug(segmentation fault) about Mysql2::Statement#fields.

environment

  • ruby 2.4.1
  • mysql 5.7.16
  • mysql2 0.4.6
  • MacOS 10.12.5

example

This is probably minimal example.

stmt = client.prepare("SELECT * FROM hoge where id = '?'")
puts stmt.fields # => work!

stmt = client.prepare("INSERT INTO hoge(blah) VALUES (?)")
puts stmt.fields # => segmentation fault

I think that it doesn't be passed the expected value for mysql_fetch_fields because mysql_stmt_result_metadata returns NULL for INSERT, UPDATE, DELETE etc.
(ref: https://dev.mysql.com/doc/refman/5.7/en/mysql-stmt-result-metadata.html)

And therefore, in case metadata is NULL, I change fields function to return an empty array.
Please review this 🙏

@sodabrew
Copy link
Collaborator

Good find! I wonder if the right answer is nil instead of []. The consistent data type feels reasonable to me, I just want to make sure it's not losing some important distinction between "no fields" and "not applicable to this query type".

@sodabrew sodabrew added this to the 0.4.7 milestone Jun 22, 2017
@@ -329,6 +329,7 @@ def stmt_count
before(:each) do
@client.query "USE test"
@test_result = @client.prepare("SELECT * FROM mysql2_test ORDER BY id DESC LIMIT 1").execute
@client.query 'CREATE TABLE IF NOT EXISTS fieldsTest (blah varchar(10))'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't need a new table, you can insert into the same mysql2_test table.

Copy link
Author

Choose a reason for hiding this comment

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

thanks! I fixed it.

@kanata2
Copy link
Author

kanata2 commented Jun 22, 2017

Thank you for your review!

I just want to make sure it's not losing some important distinction between "no fields" and "not applicable to this query type".

I think so too.
I think that in case of "no fields", it should return [], but in case of "not applicable query type", it should probably raise an exception.
However, I did not know how to distinguish whether given queries are applicable or not...
What do you think?

kanata2 added 2 commits June 23, 2017 14:15
Use `!metadata` instead of `metadata == NULL`.
sodabrew added a commit to sodabrew/mysql2 that referenced this pull request Jun 28, 2017
@sodabrew
Copy link
Collaborator

I spent some time with this, and decided that nil should be the return type – "no fields" is not valid in SQL as far as I can tell, because you can't write a SELECT without fields, nor can you create a table without columns.

There were a few other issues to address, so I've committed a similar but broader change and credited you with the initial fix. Thank you!

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