[2.1] Throw exception when empty inputs found

[sharidas]

When input retrieved from stream_get_contents
is empty string, then it would be safe to return
from the parse and expect methods.

Signed-off-by: Sujith H [email protected]

[sharidas]

Description

In one of the scenario, its been found that $input is an empty string ''. This is noticed in method, parse() of Service Class. When $input becomes an empty string, the call to $result = $r->parse(); causes an infinite loop https://github.com/sabre-io/xml/blob/2.1/lib/Reader.php#L63-L69.
So in this PR, I have checked if the value of $input is an empty string, then return from the method.
I have added this check in the second method in Service class expect(), where a similar code style is found.

Let me know if this changeset looks ok or not.

[DeepDiver1975]

THX @sharidas

Can I ask you to add unit tests which test this behavior - and if possible show the infinite loop if the change is not applied?

THX

[evert]

I think this is a good report, but aside from having a test I have 2 additional issues with this:

1. I think the issue should be solved on a lower level, e.g. Reader instead of Service. Ideally the problem gets fixed as close to the source as possible.
2. The solution to this problem should be to throw an exception, not return an empty string. I think a completely empty doc is probably always an error, and silent recoveries are dangerous.

[codecov]

Codecov Report

Merging #166 into 2.1 will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.1     #166      +/-   ##
============================================
+ Coverage     97.49%   97.51%   +0.02%     
- Complexity      112      114       +2     
============================================
  Files            13       13              
  Lines           439      443       +4     
============================================
+ Hits            428      432       +4     
  Misses           11       11

Impacted Files	Coverage Δ	Complexity Δ
lib/Service.php	100% <100%> (ø)	20 <0> (+2)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49c1b91...1c90dcc. Read the comment docs.

[sharidas]

I have updated the unit test for this change. I have made a minor change to the previous version:

• Throw ParseException ( let me know if this looks ok? )

The problem I see when handling this issue with the Reader class is, that the details when parsed would become empty for the https://github.com/sabre-io/xml/blob/2.1/lib/Reader.php#L63. Any pointers here how to handle it from the Reader, would be appreciated. Infact its a better approach.

Regarding how the infinite loop can happen:

• Consider the snippet of code when added to the ServiceTest file:

    function dummyErrorHandler($number, $message, $file, $line) {
        return;
    }

    function testInfiniteLoop() {
        $resource = fopen('php://input', 'r');
        $util = new Service();
        set_error_handler([$this, 'dummyErrorHandler']);
        $util->parse($resource, "/sabre.io/ns");
    }

• Remove the new exception code added in the Service, i.e :

            if ($input === '') {
                throw new ParseException("The input element to parse is empty. Do not attempt to continue parsing");
            }

Now when the test is run, the code from method testInfiniteLoop would go into infinite loop in the Reader's parse().

[sharidas]

@DeepDiver1975 @evert any suggestion on #166 (comment)?

[evert]

Sounds reasonable, I'm good with this approach

[DeepDiver1975]

fine for me as well - please make sure prettyci and travis are green and we can merge.

Please also have a look if these fixes need to be applied to other branches as well - mainly master.
Older versions are out of scope from my pov

[sharidas]

While adjusting the changes for the tests, I have removed the travis configuration for php 7.0. There is an issue noticed with the syntax, incompatible. It was throwing error

TypeError: Return value of Sabre\Xml\ContextStackTest::setUp() must be an instance of Sabre\Xml\void, none returned

with the current change in php 7.0.

Please also have a look if these fixes need to be applied to other branches as well - mainly master.

Yes thes changes can be applied to the master branch as well.

Regarding PrettyCI, there are lot of issues popped up. And many of them are out changes which are not caused due to this PR. I was wondering if we could take it as a separate pr to make prettyCI green.

[DeepDiver1975]

I will take Care from Here on. Thx so far 👍

[DeepDiver1975]

@sharidas please remove your last commit - we need to keep supporting php 7.0

I will get ci green with #167

[sharidas]

please remove your last commit - we need to keep supporting php 7.0

• Removed my last commit

[DeepDiver1975]

@sharidas please rebase to get ci green - THX

[sharidas]

@sharidas please rebase to get ci green - THX

Done. The CI is green now.