6 ms·

I found and fixed a bug in PHP's standard library

167 points by miguelxpn 7y ago

kinow 7y ago
Thanks for sharing it! Really interesting, and well done!
glofish 7y ago
Notably, the bugfix made PHP maintainers realize that the header handler had additional behaviors that they did not understand nor had a rationale for, so those behaviors were removed as well: https://github.com/php/php-src/pull/5201 https://github.com/php/php-src/pull/5201
- pacaro 7y ago
  Feels like this could be a case where DRY could fix a bug in one place
  - kichik 7y ago
    I was thinking the same so I checked and they removed the duplication: https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794... They also fixed the whitespace handling that let something like "RandomHeader: hello host:8080" mistakenly set the flag. https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60bf2609fde113d12d235#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60b...
    - pacaro 7y ago
      That looks so much better to me. The cognitive load is significantly reduced
  - sjwright 7y ago
    DRY is a good principle, but sometimes I’d rather repeat myself a little bit than build more bug-prone scaffolding to avoid repetition—especially when the repetition isn’t line-for-line identical. Just remember to always cite the repetition in comments.
    - fiddlerwoaroof 7y ago
      I’ve never come across a bug due to application of DRY, but I have seen many bugs because of “harmless” duplication resulting in inconsistent code changes several months later. Even aside from DRY, extracting a block of code to a function or method gives you an opportunity to name the block of code, which often significantly clarifies the intent of the code so, I’ve always tried to error on the side of overly DRY
      - smichel17 7y ago
        I haven't seen bugs due to DRY, but I've caused difficulty of adding new features and poor maintainability because of it. When I first took over maintaining Red Moon, I was a very new dev and went a little DRY crazy. In particular, there's a state machine for the different filter states (running, paused, stopped, etc), that had a bunch of classes (one per state) that had a lot of overlap. I pulled some common behaviors out into a supertype that the classes with those behaviors now inherited from. Except, it turns out some of those states ought to act differently (automatic pauses in secure apps should disable the overlay but not restore the backlight; manual pauses should do both), and now it's going to be extra work untangling the various states and pause logic. If I'd left the state machine (amongst other bits) as it was, this feature/bugfix would be implemented already. Also, the current state of things (pun intended) is a bit less readable, in my opinion.
        fiddlerwoaroof 7y ago
        I’ve never really understood this, because undoing DRY is relatively easy: you either copy the new function/class and rename it or you inline it in the mistaken case and adjust the code to match.
Lazare 7y ago
Interesting bug, but even better it's nice to see encouragement of people to get involved in open source. The walk through of the process was great.
- miguelxpn 7y ago
  Yeah! Before I started contributing I always thought that I wasn't good enough to even try. When I started I noticed how welcoming the communities usually are and that motivated me to contribute me even more. Nowadays I try to motivate people whenever I can.
- kalecserk 7y ago
  Humble and didactic, totally agree that we should thank the author. I for one felt compelled to contribute to OS after reading the article.
osrec 7y ago
Looking at lines 460-521 in the modified file (https://github.com/miguelxpn/php-src/blob/f4b2089b642d504be358b06bbae81fa688d8bf0e/ext/standard/http_fopen_wrapper.c#L460 https://github.com/miguelxpn/php-src/blob/f4b2089b642d504be3...), is there not a benefit to `break` out of the while loop in the nested if statements? Otherwise, it looks like it will call strstr() one extra time, even though you may have already determined that the specific header is present.
- miguelxpn 7y ago
  Good catch! That's indeed the case. Another commit was made where that piece of code was refactored into a function and it returns 1 in case the header is present so strstr isn't being called an extra time in the current code. [1] https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794...
  - osrec 7y ago
    Ah, much cleaner!
Jaxan 7y ago
I feel like this bug should be solved by using a proper parsing, instead of greedily looking for a field...
- homero 7y ago
  Headers are just text mixed with body and hard to figure out where they stop
  - speps 7y ago
    Headers stop when you have 2 line endings consecutively[1], I don't see why it's hard. [1] https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.1 https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4....
  - londons_explore 7y ago
    Easy. They stop after \r\n\r\n. There is no other RFC compliant way to end headers.
wolco 7y ago
That ends suddenly.
kazinator 7y ago
while ((s = strstr(s, "host:"))) { if (s == t || *(s-1) == '\r' || *(s-1) == '\n' || *(s-1) == '\t' || *(s-1) == ' ') { have_header |= HTTP_HEADER_HOST; } s++; } This s++ could be s + sizeof "host:" - 1. Reason being: if you have just found "host:" at address s, you will not find another one at s+1, s+2, s+3, s+4 or s+4; "host:" supports no overlapped matches. Actually since, we are really looking for "host:" preceded by whitespace, we can skip by just sizeof "host", (five bytes). Because if s points at "host:host:", the second "host:" is not of interest; it is not preceded by a whitespace character; we won't be setting the HTTP_HEADER_HOST bit for that one. Also, once we set the HTTP_HEADER_HOST bit, we can break out of the loop; there is no value in continuing it. The point of the loop is not to have a false negative: not to stop on a false match on "host:" which doesn't meet the HTTP_HEADER_HOST conditions, and thereby fail to find the real one later in the string. If we find the real one, we are done. By the way, this test shows how verbose of a language C is: *(s-1) == '\r' || *(s-1) == '\n' || *(s-1) == '\t' || *(s-1) == ' ' If we could use Javascript, look how nice it would be: strchr("\r\n\t ", s[-1])

167 points by miguelxpn 7y ago

Thanks for sharing it! Really interesting, and well done!

Notably, the bugfix made PHP maintainers realize that the header handler had additional behaviors that they did not understand nor had a rationale for, so those behaviors were removed as well: https://github.com/php/php-src/pull/5201 https://github.com/php/php-src/pull/5201

pacaro 7y ago
Feels like this could be a case where DRY could fix a bug in one place
- kichik 7y ago
  I was thinking the same so I checked and they removed the duplication: https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794... They also fixed the whitespace handling that let something like "RandomHeader: hello host:8080" mistakenly set the flag. https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60bf2609fde113d12d235#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60b...
  - pacaro 7y ago
    That looks so much better to me. The cognitive load is significantly reduced
- sjwright 7y ago
  DRY is a good principle, but sometimes I’d rather repeat myself a little bit than build more bug-prone scaffolding to avoid repetition—especially when the repetition isn’t line-for-line identical. Just remember to always cite the repetition in comments.
  - fiddlerwoaroof 7y ago
    I’ve never come across a bug due to application of DRY, but I have seen many bugs because of “harmless” duplication resulting in inconsistent code changes several months later. Even aside from DRY, extracting a block of code to a function or method gives you an opportunity to name the block of code, which often significantly clarifies the intent of the code so, I’ve always tried to error on the side of overly DRY
    - smichel17 7y ago
      I haven't seen bugs due to DRY, but I've caused difficulty of adding new features and poor maintainability because of it. When I first took over maintaining Red Moon, I was a very new dev and went a little DRY crazy. In particular, there's a state machine for the different filter states (running, paused, stopped, etc), that had a bunch of classes (one per state) that had a lot of overlap. I pulled some common behaviors out into a supertype that the classes with those behaviors now inherited from. Except, it turns out some of those states ought to act differently (automatic pauses in secure apps should disable the overlay but not restore the backlight; manual pauses should do both), and now it's going to be extra work untangling the various states and pause logic. If I'd left the state machine (amongst other bits) as it was, this feature/bugfix would be implemented already. Also, the current state of things (pun intended) is a bit less readable, in my opinion.
      - fiddlerwoaroof 7y ago
        I’ve never really understood this, because undoing DRY is relatively easy: you either copy the new function/class and rename it or you inline it in the mistaken case and adjust the code to match.

Lazare 7y ago

Interesting bug, but even better it's nice to see encouragement of people to get involved in open source. The walk through of the process was great.

miguelxpn 7y ago
Yeah! Before I started contributing I always thought that I wasn't good enough to even try. When I started I noticed how welcoming the communities usually are and that motivated me to contribute me even more. Nowadays I try to motivate people whenever I can.
kalecserk 7y ago
Humble and didactic, totally agree that we should thank the author. I for one felt compelled to contribute to OS after reading the article.

osrec 7y ago

Looking at lines 460-521 in the modified file (https://github.com/miguelxpn/php-src/blob/f4b2089b642d504be358b06bbae81fa688d8bf0e/ext/standard/http_fopen_wrapper.c#L460 https://github.com/miguelxpn/php-src/blob/f4b2089b642d504be3...), is there not a benefit to `break` out of the while loop in the nested if statements? Otherwise, it looks like it will call strstr() one extra time, even though you may have already determined that the specific header is present.

miguelxpn 7y ago
Good catch! That's indeed the case. Another commit was made where that piece of code was refactored into a function and it returns 1 in case the header is present so strstr isn't being called an extra time in the current code. [1] https://github.com/php/php-src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7 https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794...
- osrec 7y ago
  Ah, much cleaner!

Jaxan 7y ago

I feel like this bug should be solved by using a proper parsing, instead of greedily looking for a field...

homero 7y ago
Headers are just text mixed with body and hard to figure out where they stop
- speps 7y ago
  Headers stop when you have 2 line endings consecutively[1], I don't see why it's hard. [1] https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.1 https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4....
- londons_explore 7y ago
  Easy. They stop after \r\n\r\n. There is no other RFC compliant way to end headers.

wolco 7y ago

That ends suddenly.

kazinator 7y ago

while ((s = strstr(s, "host:"))) { if (s == t || *(s-1) == '\r' || *(s-1) == '\n' || *(s-1) == '\t' || *(s-1) == ' ') { have_header |= HTTP_HEADER_HOST; } s++; } This s++ could be s + sizeof "host:" - 1. Reason being: if you have just found "host:" at address s, you will not find another one at s+1, s+2, s+3, s+4 or s+4; "host:" supports no overlapped matches. Actually since, we are really looking for "host:" preceded by whitespace, we can skip by just sizeof "host", (five bytes). Because if s points at "host:host:", the second "host:" is not of interest; it is not preceded by a whitespace character; we won't be setting the HTTP_HEADER_HOST bit for that one. Also, once we set the HTTP_HEADER_HOST bit, we can break out of the loop; there is no value in continuing it. The point of the loop is not to have a false negative: not to stop on a false match on "host:" which doesn't meet the HTTP_HEADER_HOST conditions, and thereby fail to find the real one later in the string. If we find the real one, we are done. By the way, this test shows how verbose of a language C is: *(s-1) == '\r' || *(s-1) == '\n' || *(s-1) == '\t' || *(s-1) == ' ' If we could use Javascript, look how nice it would be: strchr("\r\n\t ", s[-1])