Fix GH-12251: Add semicolon to end of odbc dsn

[SakiTakamachi]

closes #12251

It seems that if the $dsn ends with a quote, it causes a connection error, so I added a semicolon at the end.

Any further modifications should be made to master, so I won't cover them in this PR, but I have some doubts about the specification where the second and third arguments are ignored under certain conditions.

[SakiTakamachi]

@NattyNarwhal
May I request a review?

[SakiTakamachi]

@Girgias
I saw you commenting on the original thread, so if you don't mind, could you take a look?

[staabm]

can it be unit tested?

[SakiTakamachi]

@staabm
Since it concerns authentication information, testing complex patterns is extremely difficult.
If it's something simple that allows me to see if I can fix the citation error, I'll try it.

[SakiTakamachi]

If the password was an empty string, it might not work properly. I'll fix it.

[NattyNarwhal]

Dumping thoughts here, apologies if a bit scattershot, I just woke up:

• If you change ODBC, you also need to change PDO_ODBC.
• In the issue, you mentioned putting the username and password in the connection string (with a username and password also passed, but it should scan for uid/pwd in the connection string and ignore the ones passed outside of it). What happens if you quote it versus not quoting it manually?
• Quoting is indeed caused by ( characters, see the function that does it. I had consulted the ODBC documentation, as seen, but it should just simply wrap the password string in {} if there's no } characters, so it should basically just look like {foo(bar(}. Quoted passwords at the end of the connection string should work, at least with the IBM i Db2 and MariaDB ODBC drivers I was testing with at the time.
  • Perhaps the spec is wrong for passwords, but considering I needed to quote a password with a } and ; character before, it does seem needed. Though perhaps the number of characters it needs to be done for is smaller than the spec says? It shouldn't hurt though...
  • This makes me curious if it could be a bug in the FreeTDS ODBC driver? I don't know how consistent connection string parsing is between ODBC drivers. Does the driver manager do the parsing, or does the driver roll its own?
  • If it is a bug there, we should test outside the context of PHP (confirm it is) and file upstream. Regardless, we should probably keep the workaround, and mention why in a comment.

FWIW this patch does work for me with the IBM i Db2 and SQL Server 17 (which I ran against test suite) drivers that I can easily test with.

[SakiTakamachi]

@NattyNarwhal
Thank you for confirmation.
I'm just starting to see these codes, so my understanding is still limited, but at least I know the following.

1. If we pass an empty string for the password, the authentication information will not be passed in DSN when directing.

odbc_pconnect('Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test', 'test_user2', ''); // my env

gdb

2138				rc = SQLDriverConnect((*conn)->hdbc, NULL, (SQLCHAR *) ldb, strlen(ldb), dsnbuf, sizeof(dsnbuf) - 1, &dsnbuflen, SQL_DRIVER_NOPROMPT);
(gdb) p ldb
$1 = 0x7f45d265b240 "Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test"

2. If only one of uid or pwd is included in the dsn, the other will not be included in the built dsn.

odbc_pconnect('Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test;uid=test_user', 'test_user', 'p@ssw0rd'); // my env

gdb

2138				rc = SQLDriverConnect((*conn)->hdbc, NULL, (SQLCHAR *) ldb, strlen(ldb), dsnbuf, sizeof(dsnbuf) - 1, &dsnbuflen, SQL_DRIVER_NOPROMPT);
(gdb) p ldb
$1 = 0x7f089467f000 "Driver=FreeTDS;Server=mssql-server;Port=1433;Database=test;uid=test_user"

The root cause of the issue may indeed be caused by FreeTDS, but the two points I mentioned need to be taken into consideration in php.
(This is a problem I discovered while searching for the reason why it doesn't work. Sorry for deviating from the main topic.)

[SakiTakamachi]

I specified ODBC Driver 17 for SQL Server and it worked without any problems. This is a phenomenon caused by FreeTDS.

[SakiTakamachi]

Therefore, this PR should be closed for now. This fix is ​​not suitable.

[NattyNarwhal]

I do think this PR is still acceptable as a workaround (if it doesn't cause issues with other drivers - I don't know how some would react to a trailing semicolon, but it seems OK so far?).

[SakiTakamachi]

I'm especially concerned that Github Actions is failing to connect. Since the test is not direct, my changes should have no effect...

[SakiTakamachi]

FreeTDS fixed it.

FreeTDS/freetds#504