Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrading to version 0.10.2 makes our test fail #249

Closed
pbories opened this issue Jul 28, 2021 · 6 comments
Closed

Upgrading to version 0.10.2 makes our test fail #249

pbories opened this issue Jul 28, 2021 · 6 comments

Comments

@pbories
Copy link

pbories commented Jul 28, 2021

Hello @mikehaertl, we use pdftk for a long time in our project and it worked great until we tried to upgrade to version 0.10.2.

The Github CI fails with this message:

"Error: Process completed with exit code 137."

Any idea about what should be done ?

@mikehaertl
Copy link
Owner

Release 0.10.2 only contained a little change in how form data is processed. It should be backward compatible (see here). If that's really the problem please provide a short code example to reproduce the problem.

If not, what is the last version that worked for you? I don't know what you do in your CI tests but you could also check our relase notes if there's any suspicious change that matches your test cases: https://github.com/mikehaertl/php-pdftk/releases

Also check these two underlying packages and try to revert to older versions:

@VincentLanglet
Copy link

Release 0.10.2 only contained a little change in how form data is processed. It should be backward compatible (see here). If that's really the problem please provide a short code example to reproduce the problem.

If not, what is the last version that worked for you? I don't know what you do in your CI tests but you could also check our relase notes if there's any suspicious change that matches your test cases: https://github.com/mikehaertl/php-pdftk/releases

To identify the version causing the issue, we locked all our dependency versions:

  • With the release 0.10.1, everything worked fine.
  • By bumping just this package to 0.10.2, we end up with this error and have no clue why.

I'm not sure that the change you made are backward compatible. To me, there is an infinite loop.

foreach ($fields as $key => $value) {
    $key = $this->xmlEncode($key);
    fwrite($fp, "<field name=\"$key\">\n");
    if (!is_array($value)) {
        $value = array($value);
    }
    if (isset($value[0])) {
        // Numeric keys: single or multi-value field
        foreach($value as $val) {
            $val = $this->xmlEncode($val);
            fwrite($fp, "<value>$val</value>\n");
        }
    } else {
        // String keys: nested/hierarchical fields
        $this->writeFields($fp, $value);
    }
    fwrite($fp, "</field>\n");

Let's say I call the method with

['foo' => null]

In the foreach loop, $key = 'foo' and $value = null.
Since it's not an array, $value is transformed to. [0 => null].
isset($value[0]) return false, so we're in the else and writeField is called with [0 => null].
And then it's called again and again on this array.

I'm not sure to understand the isset check. Isn't

foreach ($fields as $key => $value) {
    $key = $this->xmlEncode($key);
    fwrite($fp, "<field name=\"$key\">\n");
    if (!is_array($value)) {
        $value = array($value);
    }
    // Numeric keys: single or multi-value field
    foreach($value as $val) {
        $val = $this->xmlEncode($val);
        fwrite($fp, "<value>$val</value>\n");
    }
    fwrite($fp, "</field>\n");

better ?

At least, if the check you want to do is really about the key 0, array_key_exists seems better than isset here.

@VincentLanglet
Copy link

@mikehaertl If I'm right, the test #250 should fail.

@tristanhall
Copy link

We also noticed infinite recursion errors happening after upgrading to version 0.10.2.
On our local development environments, where we have the most debugging info, the issue seems to be stemming from mikehaertl/php-pdftk/src/XfdfFile.php on line 195, just as @VincentLanglet reported.

@mikehaertl
Copy link
Owner

Oh, I see, sorry for the hassle. I've pushed a fix in #251 and also test for null values now. Can someone help testing please if this fixes your problems?

@VincentLanglet The thing is that we also must support nested (a.k.a. hierarchical) fields: In PDF forms a field can contain other fields, for example an Address field could contain fields Street and City. They can be passed as Address.Street and Address.City in the data array. That array is then converted into a nested array (see the comment block in XfdfFile::parseData() and end up in writeFields(). In this case the array value of Address is another array with field names Street and City as key (i.e. string keys)

The other case are multi select fields (added in 0.10.2, see #148). That's fields with multiple values. Here the value will also be an array but have numeric keys.

To differ between the two we check if a 0 key exists. But the proper way to do this is with array_key_exists() instead of isset().

mikehaertl added a commit that referenced this issue Jul 30, 2021
Issue #249 Fix infinite loop with null values in form data
@mikehaertl
Copy link
Owner

Release 0.10.3 is out and includes a fix. Thanks for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants