Regression: MultiSubnetFailover is now case-sensitive in 5.10.0

[colinodell]

Please check the FAQ (frequently-asked questions) first. If you have other questions or something to report, please address the following (skipping questions might delay our responses):

PHP version 8.1.7

PHP SQLSRV or PDO_SQLSRV version php-sqlsrv-5.10.0-1.el8.remi.8.1.x86_64

Microsoft ODBC Driver version msodbcsql17-17.8.1.2-1.x86_64

SQL Server version (Unknown)

Client operating system Rocky Linux 8.5 Docker image

Table schema (Unknown)

Problem description

The MultiSubnetFailover option no longer seems to do anything in version 5.10.0 when set to True. It does work if set to true (lowercase) or 1, though.

When connecting to a server with two IP addresses (one of which is offline) and MultiSubnetFailover is set to True, we sometimes get this error: Fatal error: Uncaught PDOException: SQLSTATE[HYT00]: [Microsoft][ODBC Driver 17 for SQL Server]Login timeout expired in /app/public/test.php. This does not occur when it's set to true or 1.

Expected behavior and actual behavior

I would expect that this extension attempts to connect to all IPs in parallel when the option is set to True, like it did in version 5.9.0 on PHP 8.0.

Instead, it only does this if we use true or 1 in the connection string.

Repro code or steps to reproduce

We ran the code below, both with and without the MultiSubnetFailover=True; option on two separate Docker images:

• One with PHP 8.1 and v5.10.0 of this extension
• One with PHP 8.0 and v5.9.0 of this extension

<?php

$pdo = new \PDO(
    'sqlsrv:server=somewhere.example.com;database=REDACTED;MultiSubnetFailover=True;app=some-test',
    'REDACTED',
    'REDACTED'
);

$pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION );

$statement = $pdo->prepare('SELECT 1');
$statement->execute();
print_r($statement->fetchAll());

somewhere.example.com is a host that resolves to two different IP addresses, one of which is offline and therefore times out. Both IPs were checked with netcat to confirm that one always accepts connections and the other always times out.

When running those four scenarios, only PHP 8.0 with MultiSubnetFailover=True; set works 100% of the time. All others only work occasionally.

We also tried running the four combinations above with strace. Here's a diff of two traces - the left shows PHP 8.0 with MultiSubnetFailover=True; and the right shows PHP 8.1 also with MultiSubnetFailover=True;:

Note how the left side shows an additional connection being made. The right side also looks virtually identical to removing the MultiSubnetFailover option on both 8.0 and 8.1, suggesting that the MultiSubnetFailover option does nothing on 8.1.

[gjcarrette]

Ignoring the SOCK_DGRAM stuff (which can be explained later) what I see is OK functionality with the SOCK_STREAM and connect having EINPROGRESS and the use of poll with multiple file descriptor fd arguments.
At least that is what I get in testing with PHP 7.4 (php-sqlsrv-5.8.0) and PHP 8.1 (php-sqlsrv-5.10.1-1.el8.remi.8.1.x86_64)
However, your PHP 8.1 test output does have only one file descriptor in the call to poll, even though it seems there is more than one IP address returned by the DNS call.

If you set up odbcinst.ini with these options you can check if the SQLDriverConnectW looks ok
[ODBC]
Trace=Yes
TraceFile=/tmp/odbc-trace.QUJHnJS0bQ.txt

I think that ODBC log an easier test artifact, along with using gdb with a breakpoint on SQLDriverConnectW to examine the arguments, because we don't have source code to the MSODBCSQL driver. I also examined the locals in the msphpsql source, and that looked ok too.

So there is something else going on. I was testing in a VM and you were testing in a Docker Container.
Also I was testing 5.10.1 and you were resting 5.10.0

[colinodell]

Upon further investigation, we found that MultiSubnetFailover works just fine if we change it from True (capitalized) to either true (lowercase) or 1.

I believe this regression may have been caused by this change to core_str_zval_is_true which no longer uses strnicmp to check the given value. Other boolean options may also be affected (untested).

The issue description and title have been updated accordingly.

[absci]

Thanks for all the comments and investigation. I'll test and revert options back to case insensitive.