Skip to content
This repository has been archived by the owner on Dec 18, 2021. It is now read-only.

parameter buffer is NULL for MYSQL_TYPE_STRING parameters! #20

Closed
juandent opened this issue May 16, 2016 · 23 comments
Closed

parameter buffer is NULL for MYSQL_TYPE_STRING parameters! #20

juandent opened this issue May 16, 2016 · 23 comments

Comments

@juandent
Copy link

Hi Roland,

Building with a debug build of MySQL I have found what I think is a bug. Looking at the following function you can verify that meta_data.bound_text_buffer.size() is 0. Or putting it another way, meta_data.bound_text_buffer.data() is NULL. This causes an assert violation inside mysql-5.7.12\mysql-5.7.12\libmysql\libmysql.c, line 4078, DBUG_ASSERT(param->buffer_length != 0);. Please see if you can confirm this...

void bind_result_t::_bind_text_result(size_t index, const char** value, size_t* len)
    {
      if (_handle->debug)
        std::cerr << "MySQL debug: binding text result at index: " << index << std::endl;

      detail::result_meta_data_t& meta_data = _handle->result_param_meta_data[index];
      meta_data.index = index;
      meta_data.len = len;
      meta_data.is_null = nullptr;
      meta_data.text_buffer = value;

      MYSQL_BIND& param = _handle->result_params[index];
      param.buffer_type = MYSQL_TYPE_STRING;
      param.buffer = meta_data.bound_text_buffer.data();
      param.buffer_length = meta_data.bound_text_buffer.size();
      param.length = &meta_data.bound_len;
      param.is_null = &meta_data.bound_is_null;
      param.is_unsigned = false;
      param.error = &meta_data.bound_error;
    }

Regards,
Juan

@juandent
Copy link
Author

You can verify this by debugging the Sqlpp11MySQLSelectTest Visual C++ project

@juandent
Copy link
Author

See line 61 of SelectTest.cpp, function void testSelectAll(sql::connection& db, int expectedRowCount)

@juandent
Copy link
Author

It may be that you have designed around this and it is not a true bug when calling to MySQL through the connector... but I thought I called it to your attention because if that is not it then I think it could bring corruption at some point -- that is, if that buffer should have non-zero length as suggested by the assert in MySQL source code.

Best regards,
Juan

@juandent
Copy link
Author

I believe I found a solution. Take a look at bind_result.cpp. line 115. Just add a vector to the buffer...

    void bind_result_t::_bind_text_result(size_t index, const char** value, size_t* len)
    {
      if (_handle->debug)
        std::cerr << "MySQL debug: binding text result at index: " << index << std::endl;

      detail::result_meta_data_t& meta_data = _handle->result_param_meta_data[index];
      meta_data.index = index;
      meta_data.len = len;
      meta_data.is_null = nullptr;
      meta_data.text_buffer = value;
// ADD NEXT LINE:
      meta_data.bound_text_buffer = std::vector<char>(254);
      MYSQL_BIND& param = _handle->result_params[index];
      param.buffer_type = MYSQL_TYPE_STRING;
      param.buffer = meta_data.bound_text_buffer.data();
      param.buffer_length = meta_data.bound_text_buffer.size();
      param.length = &meta_data.bound_len;
      param.is_null = &meta_data.bound_is_null;
      param.is_unsigned = false;
      param.error = &meta_data.bound_error;
    }

Best regards,
Juan

@rbock
Copy link
Owner

rbock commented May 16, 2016

Hi Juan,

I am confused by this. Did you actually run into a problem or do you suspect that there might be a problem one day? If you ran into a problem, please show me a minimally complete example.

As far as I understand the mysql documentation, the code is doing the right thing:

http://dev.mysql.com/doc/refman/5.7/en/mysql-stmt-fetch.html

They even write:

Invoke mysql_stmt_fetch() with a zero-length buffer for the column in question and a pointer in which the real length can be stored. Then use the real length with mysql_stmt_fetch_column().

Best,

Roland

@rbock
Copy link
Owner

rbock commented May 16, 2016

Please look at bind_result_t::next_impl(), too.

@juandent
Copy link
Author

Hi roland

Ok. I did run into a problem when running against a debug build of MySQL
There is an assert checking whether the buffer length is larger than zero. The check is only made in the debug build of MySQL
You will not see this if you build against a release MySQL which is the usual way
However the assert is present in the source code which makes me think MySQL needs a non zero length buffer at that point
As to the documentation there is a contradiction between it and the actual source code of MySQL

If you download the source code of MySQL I can point you to the exact file, function and line where this assert exists and you can judge for yourself as to if this is an error

My thinking is that since you normally use release builds this issue has gone undetected

Regards
Juan

Sent from my iPhone

On May 16, 2016, at 2:44 AM, Roland Bock notifications@github.com wrote:

Hi Juan,

I am confused by this. Did you actually run into a problem or do you suspect that there might be a problem one day? If you ran into a problem, please show me a minimally complete example.

As far as I understand the mysql documentation, the code is doing the right thing:

http://dev.mysql.com/doc/refman/5.7/en/mysql-stmt-fetch.html

They even write:

Invoke mysql_stmt_fetch() with a zero-length buffer for the column in question and a pointer in which the real length can be stored. Then use the real length with mysql_stmt_fetch_column().

Best,

Roland


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@rbock
Copy link
Owner

rbock commented May 16, 2016

Hi Juan,

Thanks for the clarification. The library should work with a debug version of MySQL, too.

On the other hand, I do not like to allocate memory needlessly. So I need to think about this a bit.

Best,

Roland

@juandent
Copy link
Author

Hi Roland,

Here is part of the MySQL code:

static my_bool setup_one_fetch_function(MYSQL_BIND *param, MYSQL_FIELD *field)
{
  DBUG_ENTER("setup_one_fetch_function");
//.....
  case MYSQL_TYPE_VAR_STRING:
  case MYSQL_TYPE_STRING:
  case MYSQL_TYPE_DECIMAL:
  case MYSQL_TYPE_NEWDECIMAL:
  case MYSQL_TYPE_NEWDATE:
  case MYSQL_TYPE_JSON:
    DBUG_ASSERT(param->buffer_length != 0);
    param->fetch_result= fetch_result_str;
    break;

this assert is the one complaining about empty buffer, it exists in line 4078 of function setup_one_fetch_function in file libmysql.c of the MySQL source code.

Please let me know if this is more clear

Regards!
Juan

@juandent
Copy link
Author

Hi Roland

If the documentation is correct then the assert is incorrect but in that case a release build should run fine since the assert is only evaluated in debug build of MySQL

If that's the case then I need to build a release build of MySQL and that's the end of it

However since documentation often diverges from implementation I would trust the assert to be right and the documentation to be outdated

Regards
Juan

Sent from my iPhone

On May 16, 2016, at 7:44 AM, Roland Bock notifications@github.com wrote:

Hi Juan,

Thanks for the clarification. The library should work with a debug version of MySQL, too.

On the other hand, I do not like to allocate memory needlessly. So I need to think about this a bit.

Best,

Roland


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@rbock
Copy link
Owner

rbock commented May 16, 2016 via email

@juandent
Copy link
Author

juandent commented May 16, 2016

The MySQL documentation talks about mysql_stmt_fetch not about mysql_stmt_bind_result

Invoke mysql_stmt_fetch() with a zero-length buffer for the column in question and a pointer in which the real length can be stored. Then use the real length with mysql_stmt_fetch_column()

The assert occurs in setup_one_fetch_function which is called from mysql_stmt_bind_result

Best regards,
Juan

@rbock
Copy link
Owner

rbock commented May 16, 2016 via email

@juandent
Copy link
Author

I apologize

Did not look at the sample code

Regards
Juan

Sent from my iPhone

On May 16, 2016, at 11:12 AM, Roland Bock notifications@github.com wrote:

Huh?

The example on that page starts with:

real_length= 0;
bind[0].buffer= 0;
bind[0].buffer_length= 0;
bind[0].length= &real_length
mysql_stmt_bind_result(stmt, bind);

So yes, it is about mysql_stmt_bind_result.

But as I wrote earlier, of course I want the code to work with the debug
version as well. If that requires non-zero length, then I need to find a
solution for that.

Best,

Roland

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@rbock
Copy link
Owner

rbock commented Sep 3, 2016

Dear Juan,

Sorry for taking so long. I finally got to this. Please check if the latest commit on develop does the trick (you will need to update sqlpp11, too).

In contrast to the solution you proposed, I am using resize if the bound_text_buffer is empty. This should result in fewer allocations.

Best,

Roland

@juandent
Copy link
Author

juandent commented Sep 5, 2016

Hi Roland!

I just tested the develop branch and found the problem is indeed corrected. Yet, there is an important define that isn't being defined in the sqlpp-mysql project: NOMINMAX. This is required to avoid conflicts with Microsoft's macros for min and max.

One question: why is resize(8) better than just assigning a new vector? What underlying implementation are you relying on...

Best regards and thanks for getting this right!

Juan Dent

@rbock
Copy link
Owner

rbock commented Sep 6, 2016

Hi Juan,

Thanks for the feedback!

About the vector: I guess I wrote a bit hastily. The conditional resize would result in less allocations if bind were called more than once per result. I don't think this is currently the case. However, if this should change for whatever reason, then resize is the better option.

About the NOMINMAX: I introduced that in the one place, where it bit me. I am not sure where to put it in general?

Best,

Roland

@juandent
Copy link
Author

juandent commented Sep 8, 2016

Hi Roland,

Concerning NOMINMAX, I only have to define it in the sqlpp-mysql project so maybe that's the place to go.

One question if you don't mind: given the quality of your sqlpp library, what do you use for GUI interface? I mean, GUI libraries like Qt are so old-fashioned when it comes to using modern C++ that it's discouraging... Maybe you have something better - or is proprietary...

Best regards,
Juan

@rbock
Copy link
Owner

rbock commented Sep 9, 2016

Hi Juan,

OK, so you mean you (and MSVC users in general) can take care of the NOMINMAX without changes in the library?

As for the GUI, I'll answer in the chat, since I guess that topic might be interesting for more people.

Best,

Roland

@juandent
Copy link
Author

juandent commented Sep 9, 2016

so you mean you (and MSVC users in general) can take care of the NOMINMAX without changes in the library?

YES, a simple #define in the preprocessor part of the sqlpp-mysql Visual Studio project

Glad to be of help!

Juan

On Sep 8, 2016, at 11:03 PM, Roland Bock notifications@github.com wrote:

Hi Juan,

OK, so you mean you (and MSVC users in general) can take care of the NOMINMAX without changes in the library?

As for the GUI, I'll answer in the chat, since I guess that topic might be interesting for more people.

Best,

Roland


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #20 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AFHnFZQm0Yy0CBy6-xiMI1nhE1pYq1Bcks5qoOiPgaJpZM4Ie-B3.

@juandent
Copy link
Author

juandent commented Sep 9, 2016

Hi Roland,

Where in Github.com will you refer to the GUI issue? This is SO important to me - and as you say, for many others!!

Best regards,
Juan

On Sep 8, 2016, at 11:03 PM, Roland Bock notifications@github.com wrote:

Hi Juan,

OK, so you mean you (and MSVC users in general) can take care of the NOMINMAX without changes in the library?

As for the GUI, I'll answer in the chat, since I guess that topic might be interesting for more people.

Best,

Roland


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #20 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AFHnFZQm0Yy0CBy6-xiMI1nhE1pYq1Bcks5qoOiPgaJpZM4Ie-B3.

@rbock
Copy link
Owner

rbock commented Sep 9, 2016

Hi Juan,

I answered in the gitter chat: https://gitter.im/sqlpp11/Lobby

Best,

Roland

@rbock
Copy link
Owner

rbock commented Feb 19, 2017

This is released now

@rbock rbock closed this as completed Feb 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants