Hello
Yesterday I've got fatal error in module cloudflare.
The reason of the error was that cloudflare returns list of IP's with symbol \n in the end.
Please look at the screenshot:
There is piece of code, from function cloudflare_ip_ranges().
You can see that we have empty value in the end of the list of IP's after execution of explode function.
Due this reason we have recursion in function cloudflare_ip_address_in_range():
1. First execution cloudflare_ip_address_in_range() - $range is empty, we will take $range from cloudflare_ip_ranges()
2. We've got $range, it's array. Foreach is executing.
3. Last item in the foreach is empty string.
4. Function cloudflare_ip_address_in_range is called for this empty string and the $range is empty again and function cloudflare_ip_ranges() will be called again. We're receiving the same list of the IP's again to check each of them and it's infinity cycle.
Comment | File | Size | Author |
---|---|---|---|
#2 | cloudflare-fatal_error_due_recursion-2605732-2-D7.patch | 479 bytes | eugene.ilyin |
Selection_010.png | 151.14 KB | eugene.ilyin |
Comments
Comment #2
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedI've prepared patch to correct this problem. We should cut this empty string to avoid recursion.
Comment #3
Arvi89 CreditAttribution: Arvi89 commentedThx man, I couldn't figure out where this recursion came from, I thought I had broken something somehow ^^
(varnish would return me an error 500 which was a bit scary at first ^^)
Comment #4
Jason Ruyle CreditAttribution: Jason Ruyle as a volunteer commentedI ran into this same problem and had to turn off cloudflare on a few of my sites. It didn't happen to all of them, but did cause some major headaches.
I'm testing this patch and so far so good.
This should be added to dev or stable branch.
Comment #5
phayes CreditAttribution: phayes commentedMerged.
Comment #6
eugene.ilyin CreditAttribution: eugene.ilyin as a volunteer and at DrupalJedi commentedThank you. Perhaps it was committed but seems not pushed yet.
Comment #7
pwaterz CreditAttribution: pwaterz commented@phayes doesn't look like you committed the patch. Also we are still seeing this issue even with this patch.
Comment #8
pwaterz CreditAttribution: pwaterz commentedWhat happens if https://www.cloudflare.com/ips-v4 goes down? There isn't any error checking for that.
Comment #10
nullkernel CreditAttribution: nullkernel as a volunteer commentedI recently downloaded and have done some testing and debugging of the currently available 7.x-2.x-dev. The patch in #2 was part of what I downloaded. I was unable to reproduce the problem with the latest code so I'm changing this issue back to "Fixed".
There should be no fatal error or infinite recursion. I tested by changing the CLOUDFLARE_URL_IPV4_RANGE constant to point to a file under my control (and commenting out the cache_set statement so that I wouldn't have to clear cache repeatedly to test). Having extra newlines in the file doesn't cause problems - they are effectively ignored. Even when the file has only newlines in it, there was no infinite recursion.
As for error checking, having the file unavailable (404) or on a nonexistent server doesn't cause problems either. When either of these cases occur, an error message gets logged: "Unable to fetch IP address information from cloudflare. Until resolved, IP address detection is non-functional". Once the server is responsive (i.e. not a 404 or otherwise causing file_get_contents to return FALSE), Drupal will cache Cloudflare's response.