Fix PDO OCI Bug #60994 (Reading a multibyte CLOB caps at 8192 chars)

[mvorisek]

based on GH-5233 (authored by Randy Stark)

/cc original authors @gschc1, @cjbj, @cmb69

[cmb69]

@cjbj, could you please review?

[cjbj]

@cmb69 I pinged our developer about it yesterday.

[mvorisek]

I have tested this PR in our CI and I can confirm it works. Can it be merged?

Test results:

• PDO_OCI driver - this PR fixes the issue https://bugs.php.net/bug.php?id=60994
• OCI8 driver - no fix needed, works /w and /wo this PR

[mvorisek]

I pinged our developer about it yesterday.

@cjbj can you please ping someone from your team again to review this PR?

[cjbj]

Pinged.

[mvorisek]

@cmb69 do you think someone else can review & merge this PR?

This bugfix PR is here for more than 50 days and the original #5233 for more than 2 years.

It also have a test and I have also tested it by myself in a real app.

Thank you in advance.

[cjbj]

It's not a matter of reviewing it; it's a matter of working out why some diffs are occurring.

[mvorisek]

@cjbj thank you for your comment, what do you mean by

some diffs are occurring

I belive the only issue is that the deprecated [1] OCILobRead function is used instead the newer OCILobRead2 which does support Unicode multibyte strings correctly.

[1] https://docs.oracle.com/cd/E11882_01/appdev.112/e10646/ociaedep003.htm#LNOCI17212

[mvorisek]

the only issue with the original code is the line

php-src/ext/pdo_oci/oci_statement.c

Line 660 in c8d47cf

self->offset += amt;

where the offset is saved

the diff above shows dumped variables with the same data from the original and the new code

The offset is in characters, but everything else is in bytes.

@cjbj is there any way how length in characters can be obtained from buffer data and bytes length? If yes, please advise, if not, I belive the code /wo OCILobRead2 (available since Oracle 10g, released in 2003) cannot be fixed.

[mvorisek]

@cmb69 can you please review with the discussion above?

[cjbj]

@mvorisek my developer is happy but is seeing a diff on the first test of single byte characters where the expected number of bytes seems out of whack.

[mvorisek]

@cjbj thank you for the feedback, can you please provide more details and advise possible fix?

[cjbj]

I believe an xfail can be removed and the size fixed. I was pointed at the updated file but it is on a system I can't access so I'm still waiting. The PR can be merged and the test fixed later.

[cjbj]

Via magic:

---XFAIL--
-Fails due to Bug 60994
 --EXPECT--
 Test 1:  j
-size of string1 is 1000006 bytes, 1000006 chars.
-size of stream1 is 1000006 bytes, 1000006 chars.
+size of string1 is 8193 bytes, 8193 chars.
+size of stream1 is 8193 bytes, 8193 chars.
 beg  of stream1 is abcjjjjjjj
 end  of stream1 is jjjjjjjxyz

[mvorisek]

I tested this PR with #8348, the pdo_oci tests are passing incl. ext/pdo_oci/tests/bug60994.phpt, can this PR be merged?

when merging, please mention also the original author of #5233 in the NEWS and also close that PR

[cjbj]

@mvorisek I have prodded development. My own time with code is limited these days :(

[mvorisek]

Thank to the CI, we can be safe now about the PR. Previsouly, the test /w 1000006 bytes was definitely wrong/xfail.

If there is no feedback to address, please merge into PHP 8.0/bugfix.

[mvorisek]

@cmb69 would it be possible to merge this PR?

[mvorisek]

@cmb69 @kocsismate can you please review this PR and merge it?

[cmb69]

I can't really comment on oci issues, but this fix seems to be okay. However, I wouldn't want to merge that into any of the stable branches (targeting PHP-8.2 should still be okay), especially since this is a long standing issue (it has been reported more than ten years ago).

So this should better be decided by the respective RMs. @sgolemon, @carusogabriel, thoughts?

And maybe @ashnazg wants to have a look, since he commented on the bug report (albeit long ago).

[mvorisek]

About the targetting: this PR fixes a major PDO OCI issue - larger than 8KiB BLOBs /w multibyte UTF-8 text cannot be currently read which almost impossible to overcome from the user perspective. I still belive this PR should target PHP 8.0. We have CI tests and it definitely fits the category bugfix.

[carusogabriel]

I will give @sgolemon the final word, but if I can drop my 2 cents:

I don't see any problems merging into PHP 8.0 - it's an isolated part of PHP, and if we have regressions - albeit the tests we have in place - it shouldn't be a catastrophe. I can't, though, access the impact - hence asking Sara's input.

[cmb69]

We have CI tests

Which only test one of the code paths (HAVE_OCILOBREAD2), to my knowledge.

[mvorisek]

@sgolemon any chance you can approve this bugfix?

[ashnazg]

+1 from me... if it's making my PHPT pass, then I'm good.

[mvorisek]

This PR has been here for a while. Initially it target PHP 8.0, it was reviewed by @cjbj and @cmb69 and release manager @carusogabriel has given it a green light. During this time PHP 8.0 development was closed, so I rebased/retargetted the PR into PHP 8.1. Now, I would kindly ask if there is anything I need to address or of this PR can be merged. Thank you.

[carusogabriel]

@mvorisek RMs from PHP 8.1 should review this now.

[mvorisek]

Hi @krakjoe, @ramsey, @patrickallaert, pinging you as you are RM for PHP 8.1.

[ramsey]

If @cjbj and @cmb69 have given it the green light, then I see no reason not to merge this into 8.1.

[cjbj]

LGTM

[ramsey]

Committed in 4df4264