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

MySQL 8: Error executing statement parameters: Incorrect arguments to mysqld_stmt_execute #64

Open
Zash opened this issue Feb 28, 2021 · 17 comments

Comments

@Zash
Copy link
Contributor

Zash commented Feb 28, 2021

Can be reproduced with e.g.

db = assert(require"DBI".Connect("MySQL", "prosody", "prosody", "secret", "127.0.0.1", 3306))
s = assert(db:prepare("SELECT 1 LIMIT ? OFFSET ?"))
assert(s:execute(1, 1))

A MySQL to test against can be brought up easily using:

docker run -d --rm --name mysql \
  -e MYSQL_ROOT_PASSWORD=secret \
  -e MYSQL_USER=prosody \
  -e MYSQL_PASSWORD=secret \
  -e MYSQL_DATABASE=prosody \
  -v mysql:/var/lib/mysql \
  -p 3306:3306 \
  mysql:8.0.23

Theory: MySQL expects integers in the LIMIT slot but LuaDBI is pushing floats, since before Lua 5.3, all numbers are.

@tacerus
Copy link

tacerus commented Jul 20, 2021

Hi,

are there any updates on this? I encounter the same issue, and it prevents some people in the Monal XMPP client from sending messages through our Prosody server (which implements luadbi).

@formaldehydeson
Copy link

I'm using a custom XMPP client that connects to a Prosody server and am affected by this same issue.

@mwild1
Copy link
Owner

mwild1 commented Jul 29, 2021

Is everyone affected using Lua 5.3?

@tacerus
Copy link

tacerus commented Jul 29, 2021

I'm running Prosody with Lua 5.1:

prosodyctl about
Prosody 0.11.8
# Lua environment
Lua version:                    Lua 5.1
Lua module search paths:
  /usr/lib64/prosody/?.lua
  /usr/local/share/lua/5.1/?.lua
  /usr/local/share/lua/5.1/?/init.lua
  /usr/local/lib64/lua/5.1/?.lua
  /usr/local/lib64/lua/5.1/?/init.lua
  /usr/share/lua/5.1/?.lua
  /usr/share/lua/5.1/?/init.lua
Lua C module search paths:
  /usr/lib64/prosody/?.so
  /usr/local/lib64/lua/5.1/?.so
  /usr/lib64/lua/5.1/?.so
  /usr/local/lib64/lua/5.1/loadall.so
LuaRocks:               Not installed
# Network
Backend: libevent epoll
# Lua module versions
lfs:            LuaFileSystem 1.7.0
libevent:       2.1.8-stable
luaevent:       0.4.4
lxp:            LuaExpat 1.3.0
socket:         LuaSocket 3.0-rc1
ssl:            0.6

@formaldehydeson
Copy link

5.2 for me


# Prosody directories
Data directory:     /var/lib/prosody
Config directory:   /etc/prosody
Source directory:   /usr/lib/prosody
Plugin directories:
  /usr/lib/prosody/modules/


# Lua environment
Lua version:                    Lua 5.2

Lua module search paths:
  /usr/lib/prosody/?.lua
  /usr/local/share/lua/5.2/?.lua
  /usr/local/share/lua/5.2/?/init.lua
  /usr/local/lib/lua/5.2/?.lua
  /usr/local/lib/lua/5.2/?/init.lua
  /usr/share/lua/5.2/?.lua
  /usr/share/lua/5.2/?/init.lua

Lua C module search paths:
  /usr/lib/prosody/?.so
  /usr/local/lib/lua/5.2/?.so
  /usr/lib/x86_64-linux-gnu/lua/5.2/?.so
  /usr/lib/lua/5.2/?.so
  /usr/local/lib/lua/5.2/loadall.so

LuaRocks:               Not installed

# Network

Backend: select

# Lua module versions
DBI:            0.7
lfs:            LuaFileSystem 1.6.3
libevent:       2.1.8-stable
luaevent:       0.4.6
lxp:            LuaExpat 1.3.0
socket:         LuaSocket 3.0-rc1
ssl:            0.7```

@formaldehydeson
Copy link

seems to be caused by an update to MySQL, does this need to be patched in luadbi as well?
sidorares/node-mysql2#1239
there are a couple workarounds posted here

@Zash
Copy link
Contributor Author

Zash commented Aug 1, 2021

@mwild1

Is everyone affected using Lua 5.3?

I think what I meant was that all numbers in Lua 5.2 are floats, so floats is what gets pushed, but MySQL wants integers here. Was my guess anyway.

@Zash
Copy link
Contributor Author

Zash commented Sep 22, 2021

sidorares/node-mysql2#1239 (comment) suggests that MySQL expects these numbers to be strings. And indeed:

s = assert(db:prepare("SELECT 1 LIMIT ? OFFSET ?"))
assert(s:execute(1, 1)) --[[
stdin:1: Error executing statement parameters: Incorrect arguments to mysqld_stmt_execute
stack traceback:
	[C]: in function 'assert'
	stdin:1: in main chunk
	[C]: in ?
]]
assert(s:execute("1", "1")) --> true

I don't even!

@ntki
Copy link

ntki commented Nov 5, 2021

One solution would be to check whether *num == floor(*num) at https://github.com/mwild1/luadbi/blob/master/dbd/mysql/statement.c#L242 and push the appropriate mysql_type based on that. But probably it won't be a good solution in every case. There is not enough info to always guess the intended/correct type.

@karolyi
Copy link

karolyi commented May 30, 2023

@sparked435
Copy link
Collaborator

https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-22.html#mysqld-8-0-22-optimizer

It was possible (although unsupported) to insert a floating point value into a LIMIT or OFFSET clause by means of a parameter (and possibly a user variable). This is now explicitly disallowed; such values must be integers.

So: Due to how the Lua C API is implemented, we have no way of knowing within the binding if the value is an integer or a float. And because we're a binding, we also don't have a way to know if a specific parameter is inside a LIMIT or OFFSET clause.

TL;DR: we're basically hosed?

@mwild1
Copy link
Owner

mwild1 commented Nov 1, 2023

lua_tointeger() ? Or am I missing something?

Edit: Never mind, I see what I was missing now. We don't know when we need to use that instead.

@mwild1
Copy link
Owner

mwild1 commented Nov 1, 2023

So in Lua 5.3+ we can probably push this burden onto the user, because Lua does differentiate between integers and non-integers itself now. So we just need to ensure we pass to MySQL as integers if(lua_isinteger(param)).

We then either drop support for Lua 5.2 and earlier, or just live with them not being able to use LIMIT/OFFSET in recent MySQL versions, or we go an extra mile and make some kind of compatibility API with a wrapper type for integers.

The second option seems the most reasonable to me.

@sparked435
Copy link
Collaborator

So we just need to ensure we pass to MySQL as integers if(lua_isinteger(param)).

I swear I spent hours looking for exactly this and never found it :) This is exactly what is needed, at least for 5.3 and newer.

or we go an extra mile and make some kind of compatibility API with a wrapper type for integers.

I've been thinking about exactly that, for the simple reason that it may be useful in other unexpected contexts, such as emulating booleans, nil/NULL impedence mismatches - or possibly future cases unforseen. The overhead shouldn't be huge, if you don't need it don't use it.

Something like: DBI.forceinteger( 123 ) - which returns a Userdata that the underlying DBI layer knows how to interpret.

But IMHO we can, and should, move on full 5.3 integer support and worry about that compatibility API later. For correctness alone implement it in the other drivers as well.

@karolyi
Copy link

karolyi commented Nov 23, 2023

Is a PR being prepared for this?

FreeBSD stopped including mysql 5.x in ports, hence I'm not even in the situation of compiling luadbi with it, so I could have mysql support for prosody.

https://www.freshports.org/databases/mysql57-client

@sparked435
Copy link
Collaborator

For what it's worth, I can confirm MariaDB 10.6 is not impacted by this.

(I'm fully aware that doesn't mean much to a lot of people who have their reasons to use mainline MySQL.)

sparked435 added a commit that referenced this issue Mar 1, 2024
Attempt to provide partial fix for Issue #64.
@sparked435
Copy link
Collaborator

Finally managed to test #75 against MySQL 8.0.36 with Lua 5.4 and it worked flawlessly.

I'm reluctant to close this issue without further confirmation. Can someone who originally reported trouble, ie with Prosody, test and see if the problem is fixed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants