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

Fixed infinite loop for char "&" in unquoted attribute #28

Merged
merged 2 commits into from
Feb 11, 2014
Merged

Fixed infinite loop for char "&" in unquoted attribute #28

merged 2 commits into from
Feb 11, 2014

Conversation

miso-belica
Copy link
Contributor

I discovered that my server was overloaded by Apache's threads because of infinite loops in this library. Reproducible example:

<?php
require __DIR__ . '/vendor/autoload.php';

// echo '<xmp>';
$dom = \HTML5::loadHTML(file_get_contents('test.html'));
echo HTML5::saveHTML($dom);

And file test.html

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8" />
</head>
<body>
  <a href=index.php?str=1&id=29>Link</a>
</body>
</html>

There are more pages that is causing overload of my server so I checked some of them and noticed it is caused by character ampersand (no matter if it's valid entity or not) in unquoted attribute. It's possible that some another infinite loop in library exists, but I can't check all of the pages. If I discover another one infinite loop I create an issue at least :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.21%) when pulling a1e7f33 on miso-belica:fix-infinite-cycle into 3b69183 on Masterminds:master.

@miso-belica
Copy link
Contributor Author

Hmm, updating of composer failed. But I tested it on PHP v5.4.16 so probably it's fine on 5.4 version :)

@mattfarina
Copy link
Member

@miso-belica thanks for catching this.

I re-ran the test and it passed. I'd like @technosophos to take a look at this.

technosophos added a commit that referenced this pull request Feb 11, 2014
Fixed infinite loop for char "&" in unquoted attribute
@technosophos technosophos merged commit 890cb84 into Masterminds:master Feb 11, 2014
@technosophos
Copy link
Member

Yup, I agree with this patch. I'm merging it. Thanks, @miso-belica

@miso-belica miso-belica deleted the fix-infinite-cycle branch February 11, 2014 17:34
@technosophos
Copy link
Member

@miso-belica Just wanted to let you know -- I liked the way you presented the issue here very much, and I'm using this issue as an example to my CompSci students for how to contribute to someone's project. Thanks again.

@miso-belica
Copy link
Contributor Author

Yeeeeeee, I will be famous :D

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

Successfully merging this pull request may close these issues.

None yet

4 participants