Closed sharidas closed 5 years ago
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.
I think this is a good report, but aside from having a test I have 2 additional issues with this:
Merging #166 into 2.1 will increase coverage by
0.02%
. The diff coverage is100%
.
@@ 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) |
:arrow_up: |
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.
I have updated the unit test for this change. I have made a minor change to the previous version:
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");
}
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()
.
@DeepDiver1975 @evert any suggestion on https://github.com/sabre-io/xml/pull/166#issuecomment-518564299?
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
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.
I will take Care from Here on. Thx so far 👍
@sharidas please remove your last commit - we need to keep supporting php 7.0
I will get ci green with #167
please remove your last commit - we need to keep supporting php 7.0
@sharidas please rebase to get ci green - THX
@sharidas please rebase to get ci green - THX
Done. The CI is green now.
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 sharidasan@owncloud.com