Fix GH-12296: [odbc] [pdo_odbc] Optimized odbc connection string creating #12306

SakiTakamachi · 2023-09-27T08:46:54Z

(edit) I also made a similar fix for pdo.

I'm wondering if I should make these changes to PHP 8.2.

With this change, several use cases such as those shown below can be handled successfully.

Correct credentials (my env):

uid: test_user2
pwd: ''

Cases where the connection string contains only either uid or pwd:

odbc_pconnect('Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test;uid=test_user2', 'hoge', '');
odbc_pconnect('Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test;pwd=', 'test_user2', 'fuga');

Cases where both uppercase and lowercase letters are used in attribute keys:

odbc_pconnect('Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test;uiD=test_user2;Pwd=', 'hoge', 'fuga');

cc: @NattyNarwhal

If we don't need a password, we shouldn't include it in our dsn. However, if the password is set to an empty string, we should include it in the dsn.

So to be able to differentiate between them, I changed it to make pwd nullable and not include it in dsn if null is specified.

If uid is included in dsn, the uid argument is completely useless, so I also changed uid to a similar specification.

nielsdos

Without looking into the semantics as of now, there is something that caught my eye

ext/odbc/php_odbc.c

SakiTakamachi · 2023-09-28T12:32:27Z

@NattyNarwhal

The corrections to the indicated items have been completed. Please confirm this.

NattyNarwhal · 2023-09-28T16:54:37Z

The Db2i driver complains with PWD= at the end (though it seems fine at the middle like ;PWD=;:

Warning: odbc_connect(): SQL error: [IBM][System i Access ODBC Driver]Syntax error in connection string., SQL state S1000 in SQLConnect in /Users/calvin/src/test.php on line 8

It generates a connection string like this:

"Driver=IBM i Access ODBC Driver;System=XXX;CCSID=1208;AlwaysCalculateResultLength=1;UID=calvinb;PWD="

Empty PWD= at the end seems to cause a syntax error with that driver. Manually adding UID=calvinb;PWD={} (so that it's quoted) works though for an empty password (though it would say, override the password set if you were doing DSN= - w/ the Db2i driver, this could be useful because on i a *LOCAL DSN is premade in odbc.ini with PWD=*CURRENT set):

"Driver=IBM i Access ODBC Driver;System=XXX;CCSID=1208;AlwaysCalculateResultLength=1;UID={calvinb};PWD={}"

Warning: odbc_connect(): SQL error: [IBM][System i Access ODBC Driver]Communication link failure. comm rc=8002 - CWBSY0002 - Password for user CALVINB on system XXX is not correct, Password length = 0, Prompt Mode = Never, System IP Address = XXX, SQL state 28000 in SQLConnect in /Users/calvin/src/test.php on line 8

I think if the password is empty, it might be best to not include PWD=. The same could be done for UID=, so a username can be passed and appended, but an empty password wouldn't (And vice versa, but I don't know how useful setting password without a username could be. Maybe in the DSN= case again?). Alternatively, there could be a difference in behaviour between null and an empty string, where null wouldn't append, but an empty string would append PWD={} or whatever. That seems a little more complicated though.

(Also, changes should be made to PDO_ODBC too, since the logic is really similar there.)

SakiTakamachi · 2023-09-28T22:40:43Z

Thank you.

The Db2i driver complains with PWD= at the end (though it seems fine at the middle like ;PWD=;:

It works with Sql server and FreeTDS, so it seems like a problem with Db2i.

However, over the past few days, I have noticed that there are many cases where the detailed specifications of ODBC are slightly different, and I agree that it should be made as secure as possible.

I think if the password is empty, it might be best to not include PWD=.

If you do not set pwd when the password is empty, authentication will probably fail. It also expects the argument to be a string, so it won't pass null for reasons other than bugs.

(Also, changes should be made to PDO_ODBC too, since the logic is really similar there.)

ok, as soon as the policy is finalized, I will revise the PDO as well.

I still don't understand it properly, but what kind of connection string did Db2i use to successfully authenticate?

NattyNarwhal · 2023-09-29T19:44:56Z

It works with Sql server and FreeTDS, so it seems like a problem with Db2i.

However, over the past few days, I have noticed that there are many cases where the detailed specifications of ODBC are slightly different, and I agree that it should be made as secure as possible.

Yeah, finding out that drivers do parsing their own way every time is really annoying. The worst is when we're getting into a situation where two drivers basically do something where we have to have mutually exclusive behaviour for dealing with them.

It almost makes me wish the connection string + appended username and password case was deprecated, and the interface matched ODBC's with a DSN+UID+PWD triplet or a connection string. It'd be a big breaking change though.

If you do not set pwd when the password is empty, authentication will probably fail. It also expects the argument to be a string, so it won't pass null for reasons other than bugs.

I think this might be driver specific, but again, there is the DSN= behaviour, where you could pick up a default username and/or password from a defined data source, and override it. In that case, setting an empty password may not be ideal over using the existing password.

I still don't understand it properly, but what kind of connection string did Db2i use to successfully authenticate?

For defining an empty password, PWD={} is validly parsed regardless of the position of PWD in the connection string (though I'm sure the system won't let you log in with a blank password).

SakiTakamachi · 2023-09-30T01:31:50Z

It almost makes me wish the connection string + appended username and password case was deprecated, and the interface matched ODBC's with a DSN+UID+PWD triplet or a connection string. It'd be a big breaking change though.

For defining an empty password, PWD={} is validly parsed regardless of the position of PWD in the connection string (though I'm sure the system won't let you log in with a blank password).

My concern is that if you actually set the password to an empty string, you won't be able to authenticate unless you pass pwd= in the connection string.

I'm currently setting up a docker environment to see how Db2i behaves.

For reference, with SQL Server, it is possible to set an empty string for the password, although it requires some effort, so there is clearly a use case for this.

I'm sorry if I misunderstood your point and said something off-topic.

NattyNarwhal · 2023-10-02T14:59:06Z

I'm currently setting up a docker environment to see how Db2i behaves.

Db2i requires an IBM i server, but you can use a public one like PUB400. Db2 LUW is a separate animal, but it'd be interesting to see how that one behaves (since I have to deal with it for CI, and it's different from Db2i).

For reference, with SQL Server, it is possible to set an empty string for the password, although it requires some effort, so there is clearly a use case for this.

That makes sense, though it annoyingly does conflict with "inherit the password from the DSN" case. If we want to have both cases, I'm thinking back to my previous comment of having null be different from an empty string (though I think a lot of existing code conflates the two?).

I'm sorry if I misunderstood your point and said something off-topic.

It's not a problem. The drivers aren't terribly consistent, so it's easy to get lost in the weeds.

SakiTakamachi · 2023-10-02T15:24:02Z

Db2i requires an IBM i server, but you can use a public one like PUB400. Db2 LUW is a separate animal, but it'd be interesting to see how that one behaves (since I have to deal with it for CI, and it's different from Db2i).

That's very useful information. Thank you!

That makes sense, though it annoyingly does conflict with "inherit the password from the DSN" case. If we want to have both cases, I'm thinking back to my previous comment of having null be different from an empty string (though I think a lot of existing code conflates the two?).

Looking at the signatures of odbc_connect() and odbc_pconnect(), it seems that null is never passed. Is it possible that nulls are passed in calls other than these functions?

It's not a problem. The drivers aren't terribly consistent, so it's easy to get lost in the weeds.

Thank you. I've already gotten lost on this issue many times...

NattyNarwhal · 2023-10-02T15:50:03Z

Looking at the signatures of odbc_connect() and odbc_pconnect(), it seems that null is never passed. Is it possible that nulls are passed in calls other than these functions?

Sorry, I was thinking of both odbc in PHP 7, as well as PDO right now (which it has a nullable string in the connection constructor for uid/pwd explicitly).

SakiTakamachi · 2023-10-02T16:20:20Z

Sorry, I was thinking of both odbc in PHP 7, as well as PDO right now (which it has a nullable string in the connection constructor for uid/pwd explicitly).

I see now, no problem.

At least, it seems like we don't have to worry about null on the master branch?
(pdo is different)

SakiTakamachi · 2023-10-04T13:04:00Z

The Db2i driver complains with PWD= at the end (though it seems fine at the middle like ;PWD=;:

Warning: odbc_connect(): SQL error: [IBM][System i Access ODBC Driver]Syntax error in connection string., SQL state S1000 in SQLConnect in /Users/calvin/src/test.php on line 8
It generates a connection string like this:

"Driver=IBM i Access ODBC Driver;System=XXX;CCSID=1208;AlwaysCalculateResultLength=1;UID=calvinb;PWD="

It took a lot of effort, but I finally got the db2i environment set up and reproduced the behavior you described.
It may be a good idea to always add a semicolon at the end of the DSN, also as a workaround for older versions of FreeTDS.

SakiTakamachi · 2023-10-10T14:56:35Z

I also confirmed the operation of db2 LUW. db2 LUW did not exhibit any strange behavior.
Therefore, considering the verification results of other DBs, I decided to add a semicolon at the end.

Operation confirmed

FreeTDS
SQLServer
db2 for i
db2 LUW

SakiTakamachi · 2023-10-11T03:45:19Z

If there is a semicolon at the end of the original dsn, the semicolon will be duplicated, so I modified it so that when adding a uid or pwd, if there is a semicolon at the end, it is removed.

I also made a similar fix for pdo.

SakiTakamachi · 2023-10-11T03:50:21Z

@NattyNarwhal
All modifications have been successfully completed.
Could you confirm it?

Also, it would be helpful if you could give us your opinion on whether these changes should be made to master or 8.2.

SakiTakamachi · 2023-10-11T04:38:30Z

For pdo, it was necessary to take null into account. I'll fix it later.

SakiTakamachi · 2023-10-11T05:02:43Z

done.

NattyNarwhal · 2023-10-11T15:00:04Z

Works with db2i driver; an empty password clearly states the driver is receiving an empty string as a password (and failing to authenticate, but desired behaviour for this PR).

SakiTakamachi · 2023-10-11T16:05:20Z

@NattyNarwhal

Thank you for confirmation!

Dismissing review: not relevant anymore and letting someone else review it

SakiTakamachi · 2023-10-12T04:12:52Z

@NattyNarwhal

All that's left is the problem, as you say, that passing an empty string as the password when establishing a connection that doesn't require a password causes an error.

I think this can be achieved by making the credential argument optional, like pdo, and nullable, to distinguish between empty strings and null.

Since this will require an RFC, I think it would be better to create and implement an RFC separately from this PR.

NattyNarwhal · 2023-10-12T15:01:55Z

IMHO it's a minor and isolated enough change that you probably don't need an RFC for it, but maybe others might disagree there.

nielsdos · 2023-10-12T15:31:15Z

In case of doubt, you can ask the mailing list for potential complaints and check there if an RFC is needed.

SakiTakamachi · 2023-10-13T00:24:53Z

Thank you, I tried sending an email yesterday.

SakiTakamachi · 2023-10-13T00:27:48Z

Also, I’m still looking for opinions on the target branch.

add test case fix test

SakiTakamachi · 2023-10-26T00:36:15Z

@NattyNarwhal

Changed signature to nullable and optional. Now you can generate a dsn that does not include a password.

I would be happy if you could try it when you have time.

NattyNarwhal · 2023-10-26T16:06:19Z

Works for me, the blank string and null paths both work in both extensions.

FWIW once this gets merged, probably a good idea to update the odbc_connect signature in documentation, as well as actually document what it does, since it's vague about the connection string + uid/pwd case.

(Note that IIRC, doc changes if it'll only be in the new release only get merged once the release starts to happen.)

SakiTakamachi · 2023-10-26T23:51:17Z

Thank you for confirmation!

@kocsismate

This is something we were discussing on the mailing list, could you please give me your opinion on whether master should be the target branch?

kocsismate · 2023-10-30T13:25:21Z

This is something we were discussing on the mailing list, could you please give me your opinion on whether master should be the target branch?

We very rarely change signatures in patch versions so I'd say that it's best to target master.

NattyNarwhal · 2023-10-30T13:27:29Z

Yeah, I think that makes sense. Maybe it can be backported as a bug fix later if there's demand.

SakiTakamachi · 2023-10-30T14:36:22Z

Thank you!

Also....there is no code owner for odbc and pdo_odbc, so I don't know if there is anyone else to ask for a review.

NattyNarwhal · 2023-10-30T16:37:17Z

I usually ask @cmb69 since he wrote most of the recent commits; he's also knowledge about SQL Server drivers w/ the extension though.

iluuu1994 · 2023-10-30T17:24:26Z

@NattyNarwhal Christoph is not currently active in the PHP project. I'm not sure if there's somebody else who can review this.

NattyNarwhal · 2023-10-30T17:39:59Z

That's unfortunate. I might actually be in the best shape to review ODBC things, seeing as I've been submitting patches and triaging with end-user issues.

SakiTakamachi · 2023-10-30T23:40:51Z

@NattyNarwhal

I agree with that, thanks for see this.

May I ask you to commit?

NattyNarwhal · 2023-10-30T23:41:52Z

I don't have merge access. I do a good job of making it seem like I do though 😵‍💫

SakiTakamachi · 2023-10-30T23:50:49Z

Oops, excuse me! I mistakenly thought that all members had merge privileges.😳

Contributor Tim appears to have merge permissions, it's difficult to infer permissions from the label for me.

NattyNarwhal · 2023-10-31T01:16:58Z

I think someone else can merge, yes. I gave my LGTM earlier for them. (It'll be a bit annoying having to explain the new behaviour when 8.4 rolls out for my users, but it'll just be telling people to clean up their connect calls.)

SakiTakamachi · 2023-11-01T17:22:36Z

@Girgias

If you don't mind, could you take a look at this?

Girgias

This looks fine if @NattyNarwhal is Okay with it.

Minor comments

Girgias · 2023-11-02T17:22:09Z

ext/odbc/php_odbc.c

-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sss|l", &db, &db_len, &uid, &uid_len, &pwd, &pwd_len, &pv_opt) == FAILURE) {
-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(1, 4)
+		Z_PARAM_STRING(db, db_len)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_STRING_OR_NULL(uid, uid_len)
+		Z_PARAM_STRING_OR_NULL(pwd, pwd_len)
+		Z_PARAM_LONG(pv_opt)
+	ZEND_PARSE_PARAMETERS_END();


Why not just:

if (zend_parse_parameters(ZEND_NUM_ARGS(), "s|s!s!l", &db, &db_len, &uid, &uid_len, &pwd, &pwd_len, &pv_opt) == FAILURE) {

?

Thank you for confirmation.

I read this RFC and decided that odbc_connect() is applicabled a "most often used function" when used in an application, so I thought I might as well rewrite it.
https://wiki.php.net/rfc/fast_zpp

If I'm wrong, I will rewrite it to use zend_parse_parameters.

Girgias · 2023-11-02T17:23:19Z

ext/odbc/php_odbc.c

@@ -2095,32 +2095,55 @@ int odbc_sqlconnect(odbc_connection **conn, char *db, char *uid, char *pwd, int
 		/* a connection string may have = but not ; - i.e. "DSN=PHP" */
 		if (strstr((char*)db, "=")) {
 			direct = 1;
+
+			size_t db_len = strlen(db);


Add a comment above that this should be identical to the code in the PDO driver and vice versa

Added in ad06797

SakiTakamachi · 2023-11-04T13:55:28Z

Thank you merge!

SakiTakamachi · 2023-11-04T14:48:45Z

@Girgias

I did not edit upgrade and news🙏

iluuu1994 · 2023-11-05T13:47:51Z

This change caused a regression on nightly:


========DIFF========
--
     
     dsn with correct credentials / not set user / not set password
     Connected.
018+ 
019+ =================================================================
020+ ==246894==ERROR: LeakSanitizer: detected memory leaks
021+ 
022+ Direct leak of 120 byte(s) in 5 object(s) allocated from:
023+     #0 0x7fe45bdd3587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
024+     #1 0x7fe44c0278da  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.5.1+0x1808da)
025+ 
026+ Indirect leak of 440 byte(s) in 5 object(s) allocated from:
027+     #0 0x7fe45bdd3587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
028+     #1 0x7fe44bfaf909  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.5.1+0x108909)
029+ 
030+ SUMMARY: AddressSanitizer: 560 byte(s) leaked in 10 allocation(s).
========DONE========
FAIL odbc_connect(): Basic test for odbc_connect(). (When not using a DSN alias) [ext/odbc/tests/odbc_connect_001.phpt] 

========DIFF========
--
     
     dsn with correct credentials / incorrect user / incorrect password
     Connected.
015+ 
016+ =================================================================
017+ ==252366==ERROR: LeakSanitizer: detected memory leaks
018+ 
019+ Direct leak of 96 byte(s) in 4 object(s) allocated from:
020+     #0 0x7f4276580587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
021+     #1 0x7f42667d48da  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.5.1+0x1808da)
022+ 
023+ Indirect leak of 352 byte(s) in 4 object(s) allocated from:
024+     #0 0x7f4276580587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
025+     #1 0x7f426675c909  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.5.1+0x108909)
026+ 
027+ SUMMARY: AddressSanitizer: 448 byte(s) leaked in 8 allocation(s).
========DONE========
FAIL Basic test for connection. (When not using a DSN alias) [ext/pdo_odbc/tests/basic_connection.phpt]

https://github.com/php/php-src/actions/runs/6758379815/job/18369962027

Can somebody have a look?

SakiTakamachi · 2023-11-05T16:07:38Z

@iluuu1994
From the test log, I assumed that it was probably a bug in libmsodbcsql-17.10.so.5.1, so I verified it. This prediction was correct, and when I ran the test on a DSN using FreeTDS's odbc driver, it passed.

It seems that the changes I made to the c file were completely unrelated, and the test case I added happened to cause this bug.

This problem occurs when I close and connect again, like this:

$conn = odbc_connect($dsn, $user, $pass);
if ($conn) odbc_close($conn);

$conn = odbc_connect($dsn, $user, $pass);

To make CI quiet for now, remove all closes from tests. (Tests pass even without close.)

I tried to see what would happen with msodbcsql18, but I get an error regarding TLS. This is probably a bug.

(edit)
The same is true for pdo_odbc, and if you remove $db = null; the test will pass.

SakiTakamachi · 2023-11-05T16:31:58Z

Fixed the test to avoid the bug.
#12615

NattyNarwhal · 2023-11-05T16:50:46Z

Could be another issue with the MS driver if you can't identify where it could be in PHP; I ran into that before.

SakiTakamachi · 2023-11-06T19:54:10Z

Sorry, I posted by mistake

github-actions bot added the Extension: odbc label Sep 27, 2023

SakiTakamachi changed the title ~~Optimized odbc_sqlconnect~~ Fix GH-12296: Optimized odbc_sqlconnect Sep 27, 2023

nielsdos previously requested changes Sep 27, 2023

View reviewed changes

ext/odbc/php_odbc.c Outdated Show resolved Hide resolved

SakiTakamachi force-pushed the fix/gh-12296-optimize-odbc-sqlconnect branch from d1c1e03 to 389aa3f Compare October 1, 2023 14:28

SakiTakamachi changed the title ~~Fix GH-12296: Optimized odbc_sqlconnect~~ Fix GH-12296: Optimized odbc connection string creating Oct 11, 2023

SakiTakamachi changed the title ~~Fix GH-12296: Optimized odbc connection string creating~~ Fix GH-12296: [odbc] [pdo_odbc] Optimized odbc connection string creating Oct 11, 2023

github-actions bot added the Extension: pdo_odbc label Oct 11, 2023

SakiTakamachi requested a review from kocsismate as a code owner October 25, 2023 15:01

SakiTakamachi force-pushed the fix/gh-12296-optimize-odbc-sqlconnect branch from 3c71354 to 3a50cf4 Compare October 25, 2023 15:27

Made uid and pwd of odbc_connect and odbc_pconnect nullable.

96faed4

add test case fix test

SakiTakamachi force-pushed the fix/gh-12296-optimize-odbc-sqlconnect branch from 3a50cf4 to 96faed4 Compare October 25, 2023 16:19

Girgias reviewed Nov 2, 2023

View reviewed changes

[ci skip] Added comment regarding relation to pdo

ad06797

Girgias merged commit bbe1222 into php:master Nov 4, 2023
1 check passed

SakiTakamachi mentioned this pull request Nov 5, 2023

[Tests] Avoid memory leak bug with msodbcsql #12615

Merged

SakiTakamachi deleted the fix/gh-12296-optimize-odbc-sqlconnect branch November 21, 2023 07:19

Fix GH-12296: [odbc] [pdo_odbc] Optimized odbc connection string creating #12306

Fix GH-12296: [odbc] [pdo_odbc] Optimized odbc connection string creating #12306

Conversation

SakiTakamachi commented Sep 27, 2023 • edited Loading

nielsdos left a comment

Choose a reason for hiding this comment

SakiTakamachi commented Sep 28, 2023

NattyNarwhal commented Sep 28, 2023

SakiTakamachi commented Sep 28, 2023 • edited Loading

NattyNarwhal commented Sep 29, 2023

SakiTakamachi commented Sep 30, 2023

NattyNarwhal commented Oct 2, 2023

SakiTakamachi commented Oct 2, 2023 • edited Loading

NattyNarwhal commented Oct 2, 2023

SakiTakamachi commented Oct 2, 2023 • edited Loading

SakiTakamachi commented Oct 4, 2023

SakiTakamachi commented Oct 10, 2023 • edited Loading

SakiTakamachi commented Oct 11, 2023

SakiTakamachi commented Oct 11, 2023

SakiTakamachi commented Oct 11, 2023

SakiTakamachi commented Oct 11, 2023

NattyNarwhal commented Oct 11, 2023

SakiTakamachi commented Oct 11, 2023

SakiTakamachi commented Oct 12, 2023

NattyNarwhal commented Oct 12, 2023

nielsdos commented Oct 12, 2023

SakiTakamachi commented Oct 13, 2023

SakiTakamachi commented Oct 13, 2023

SakiTakamachi commented Oct 26, 2023

NattyNarwhal commented Oct 26, 2023

SakiTakamachi commented Oct 26, 2023

kocsismate commented Oct 30, 2023

NattyNarwhal commented Oct 30, 2023

SakiTakamachi commented Oct 30, 2023

NattyNarwhal commented Oct 30, 2023

iluuu1994 commented Oct 30, 2023

NattyNarwhal commented Oct 30, 2023

SakiTakamachi commented Oct 30, 2023

NattyNarwhal commented Oct 30, 2023

SakiTakamachi commented Oct 30, 2023

NattyNarwhal commented Oct 31, 2023

SakiTakamachi commented Nov 1, 2023

Girgias left a comment

Choose a reason for hiding this comment

Girgias Nov 2, 2023

Choose a reason for hiding this comment

SakiTakamachi Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Girgias Nov 2, 2023

Choose a reason for hiding this comment

SakiTakamachi Nov 2, 2023

Choose a reason for hiding this comment

SakiTakamachi commented Nov 4, 2023

SakiTakamachi commented Nov 4, 2023

iluuu1994 commented Nov 5, 2023

SakiTakamachi commented Nov 5, 2023 • edited Loading

SakiTakamachi commented Nov 5, 2023

NattyNarwhal commented Nov 5, 2023

SakiTakamachi commented Nov 6, 2023

SakiTakamachi commented Sep 27, 2023 •

edited

Loading

SakiTakamachi commented Sep 28, 2023 •

edited

Loading

SakiTakamachi commented Oct 2, 2023 •

edited

Loading

SakiTakamachi commented Oct 2, 2023 •

edited

Loading

SakiTakamachi commented Oct 10, 2023 •

edited

Loading

SakiTakamachi Nov 2, 2023 •

edited

Loading

SakiTakamachi commented Nov 5, 2023 •

edited

Loading