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

Avoid crashing when Statement#close is called before a Result is collected #697

Merged
merged 3 commits into from
Nov 22, 2015

Conversation

sodabrew
Copy link
Collaborator

Alternative to #692 / Resolves #690

I found that you can call mysql_stmt_close first, without calling mysql_stmt_result_free, however you must not later call mysql_stmt_result_free.

@sodabrew
Copy link
Collaborator Author

@kamipo What do you think of doing it this way, rather than providing an explicit Result#free that the user must call?

@kamipo
Copy link
Contributor

kamipo commented Nov 20, 2015

I think it is better to call immediately mysql_stmt_free_result to a result set that produced by mysql_stmt_store_result. But actually this is Rubyish way, it looks good to me.
I PRed rails/rails#22352 working on this branch. It looks like works fine. Thanks!

@sodabrew
Copy link
Collaborator Author

@kamipo I asked on that Rails PR, and should probably ask here for continuity, if you require the Mysql2::Result#free method in order to hook this into the ActiveRecord adapter, or if allowing Ruby GC to handle the result free is sufficient?

sodabrew added a commit that referenced this pull request Nov 22, 2015
Avoid crashing when Statement#close is called before a Result is garbage collected
@sodabrew sodabrew merged commit 2af8e67 into brianmario:master Nov 22, 2015
@sodabrew sodabrew deleted the stmt_close_crash branch November 22, 2015 17:18
@sodabrew sodabrew mentioned this pull request Nov 22, 2015
@kamipo
Copy link
Contributor

kamipo commented Nov 23, 2015

I realized that when if (!is_streaming), Mysql2::Result#each is called in Statement#execute, then rb_mysql_result_free_result is called immediately. It seems good to me. Mysql2::Result#free is not needed for now.

https://github.com/sodabrew/mysql2/blob/ed5613d26b1f54e5eb0629a06cb5136de4ae47cf/ext/mysql2/statement.c#L374-L377

  if (!is_streaming) {
    // cache all result
    rb_funcall(resultObj, intern_each, 0);
  }

https://github.com/sodabrew/mysql2/blob/ed5613d26b1f54e5eb0629a06cb5136de4ae47cf/ext/mysql2/result.c#L840-L843

      if (wrapper->lastRowProcessed == wrapper->numberOfRows) {
        /* we don't need the mysql C dataset around anymore, peace it */
        rb_mysql_result_free_result(wrapper);
      }

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.

stmt.close cause segv
2 participants