search

FirebirdSQL / firebird

Firebird server, client and tools
https://firebirdsql.org
1.26k stars 217 forks source link

move the call of the checkArgsMismatch function before the call of th… #8218

Closed Zhdanov0 closed 2 months ago

Zhdanov0 commented 3 months ago

…e setParamsFunc function (fixed crash when calling make_dbkey function with wrong number of parameters)

First, I started to deal with the segfault that occurs when calling the make_dbkey function with zero or one parameter. For example, SELECT MAKE_DBKEY() FROM RDB$DATABASE; The solution is simple - the setParamsMakeDbkey function does not check for the number of arguments passed (if (argsCount > 1)).

I just don't understand the current logic, why checkArgsMismatch is executed after the setParamsFunc function and not before it? I think it is more logical to make the checkArgsMismatch check at the place where I put it in the PR. This location is more advantageous because there will be no need to do extra work in the form of calling the setParamsFunc function and other things if you know in advance that the number of arguments is incorrect.

Also, thanks to this location, we don't need to add the “if (argsCount > 1)” check to the setParamsMakeDbKey function, which saves us from unnecessary branching. If you are happy with this solution, I can remove these checks in the existing setParams functions. For example, you can see such branching in the setParamsPosition function.

I doubt about removing the checkArgsMismatch function call in SysFuncCallNode::pass2. I haven't found what it might affect and don't understand why it was there.

hvlad commented 3 months ago

The bug in setParamsMakeDbKey() fixed by #8221, thanks

Zhdanov0 commented 3 months ago

The bug in setParamsMakeDbKey() fixed by #8221, thanks

This PR isn't just about the make_dbkey function. Read the rest of the text too, please.

hvlad commented 3 months ago

The bug in setParamsMakeDbKey() fixed by #8221, thanks

This PR isn't just about the make_dbkey function. Read the rest of the text too, please.

Sure. I prefer to have a safe setParamsMakeDbKey() now and regardless of whether this PR gets accepted.

Zhdanov0 commented 2 months ago

Can someone please respond to the text in the PR?