IBM / node-odbc

ODBC bindings for node
MIT License
146 stars 77 forks source link

Call Procedure results in core dump if out char > 8191 #313

Closed 7rueLightSeeker closed 1 year ago

7rueLightSeeker commented 1 year ago

Hi, im working on IBM i. If i try to call a Procedure returning a char(8192) im getting following error: image In ACS it looks fine: image If i decrease the char length to 8191 or less it works.

Here is my stored Procedure: image

Is it a bug or an expected limitation?

DriesOlbrechts commented 1 year ago

I am also experiencing this issue, I assume this is a javascript limitation?

kadler commented 1 year ago

Definitely a bug. I'm surprised we're hitting an allocation failure here, since 8K is not a very big allocation (even if we multiply that for UTF-8 expansion it's still not that big). Also, since we don't have any C++ exception handlers, we really should be using new (std::nothrow).

@7rueLightSeeker Is all that's needed to recreate the issue to create a procedure with a single CHAR(8192) out param and call it? Also what platform and node version are you running on? (NOTE: we have an issue template for a reason)

DriesOlbrechts commented 1 year ago

I created a small program to reproduce this issue simply adding 1 to the VARCHAR() value will cause the error.

const odbc = require('odbc');
require('dotenv').config();
let cn = `schema=QSYSDIR;UID=${process.env.ODBC_USERNAME};PWD=${process.env.ODBC_PASSWORD};\
Database=${process.env.ODBC_DATABASE};Driver={IBM i Access ODBC Driver};\
System=${process.env.ODBC_HOST};`;
// wrap in async
async function main() {
const pool = await odbc.pool(cn);
const connection = await pool.connect();
const statement = await connection.createStatement();

await statement.prepare(`CREATE OR REPLACE PROCEDURE SCHEMA.EXAMPLE_PROCEDURE(
OUT output_param VARCHAR(8191)
)
LANGUAGE SQL
BEGIN
SET output_param = 'Hello World';
END`);
await statement.execute();
const result = await connection.callProcedure(null,'SCHEMA','EXAMPLE_PROCEDURE', ['']);
console.log(result);
}
main();
kadler commented 1 year ago

@Dries1234 Thanks, but I wasn't able to recreate. Maybe some more details would be helpful:

What platform are you running on? What Node version? What node-odbc version?

DriesOlbrechts commented 1 year ago

@kadler The code itself is running on a local WSL 2 machine (Ubuntu 22.04) The DB2 database I am accessing is on an IBM I Pase server. My node version is v18.14.0, however since this is not officially supported, I also checked using v16.9.1 The package version is ^2.4.7

Let me know if you need any more info or help locating the source of the issue.

DrunkenToast commented 1 year ago

Changing the VARCHAR(8292) also causes the issue for me, the code from @Dries1234 is reproduceable for me. Also running through WSL 2.

DriesOlbrechts commented 1 year ago
    const statement = await connection.createStatement();
    await statement.prepare(`CALL ${opts.schema}.${opts.sql_procedure}(?,?,?)`);
    await statement.bind(opts.args);
    const result = await statement.execute() as ProcResult;
    statement.close();
    return result;

I did some more testing and found a method that does not cause the issue. Calling manually has no issues, so the issue must be with the callProcedure function.

DriesOlbrechts commented 1 year ago

I was too quick to post this alternative option since we actually do not get the ouput parameter back with this method.

DriesOlbrechts commented 1 year ago

@kadler Hey there, I did some digging through the cpp code and found something that might be interesting. I can only guess however as I do not have the knowledge to know for sure.

This line here https://github.com/markdirish/node-odbc/blob/7a97292b0dcd37553f8d99b8315561d5ce3dc69c/src/odbc_connection.cpp#L1803

SQLSMALLINT is being used here. According to DB2 spec

The range of small integers is -32768 to +32767.

MAX_UTF8_BYTES is 4 So if we take our example (8191 *4) + 1 = 32765 This just barely falls in the max value

Now if we take 8192 (8192 * 4) + 1 = 32769 We just pass the limit

I could be completely off here, but I thought I'd share anyways in case I am onto something

kadler commented 1 year ago

Seems like a likely cause. I wonder why we're casting to SQLSMALLINT there.

kadler commented 1 year ago

Turns out I missed this bit:

simply adding 1 to the VARCHAR() value will cause the error.

I was able to recreate by making it a 8192. Personally, I would have found it more helpful to paste the failing code and then describe how to make it work, than vice-versa.

DriesOlbrechts commented 1 year ago

You're right, that's my bad! Could have definitely been more clear.

kadler commented 1 year ago

Looks like the cast to SQLSMALLINT was the problem, PR opened.

DriesOlbrechts commented 1 year ago

Huge thanks for the quick reaction!

kcooliver commented 1 year ago

I ran into this issue recently and this is just the fix I am looking for. Def thanks for the quick action @kadler... at this time do you have an anticipated date for inclusion in/release of 18?

kadler commented 1 year ago

@markdirish can you review #318 when you get some time?

DriesOlbrechts commented 1 year ago

Revisiting this issue for a few questions/thoughts. The new size CAP seems to be 24575 this is a huge improvement over what it was. I assume at this point we are hitting an SQL datatypes limit.

However crossing this limit still results in a core dump. Some better error handling would be nice, for now setting a hard limit does the job though. In the same category, when a parameter is set too small, it results in a segfault. Both of these would be desirable to be able to at least catch.

Either case, thanks for the quick response and solving of the issue.

eric79 commented 1 year ago

@markdirish , do you have an estimate of when your next release will be that will include this fix?

markdirish commented 1 year ago

@eric79 should be live now! Give it a test