
I found and fixed a bug in PHP's standard library - miguelxpn
https://www.miguelxpn.com/coding/php/opensource/2020/03/01/how-i-found-and-fixed-a-bug-in-php-std-lib.html
======
glofish
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
Feels like this could be a case where DRY could fix a bug in one place

~~~
sjwright
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
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
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
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.

~~~
smichel17
Easy, but it's still work. As a thought experiment: if it's so easy, would you
be interested in doing it for me? (edit: Obviously not, but compare your
internal resistance to if I'd asked you to do something trivial like fix a
typo in the readme)

Here's the file I was referencing (only 100 lines!):
[https://github.com/LibreShift/red-
moon/blob/master/app/src/m...](https://github.com/LibreShift/red-
moon/blob/master/app/src/main/java/com/jmstudios/redmoon/filter/Command.kt)

Here's the issue I've been putting off fixing because of it:
[https://github.com/LibreShift/red-
moon/issues/208](https://github.com/LibreShift/red-moon/issues/208)

The problem is that right now there is one PAUSE state, which is triggered by
both manual pauses (brightness should be restored) and automatic pauses (in
secure apps; brightness should not be restored). The problem wasn't exactly
DRY, it's the misapplication: I merged bits that had identical code, but
turned out not to have identical purpose.

And, for posterity, here's what it used to look like way back (DRYing was not
the only change, I also split out notification stuff, so it's not a 1:1 length
comparison): [https://github.com/LibreShift/red-
moon/blob/11ae916955ff8c36...](https://github.com/LibreShift/red-
moon/blob/11ae916955ff8c360eb568c3b5632673b50b58d0/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.kt)

edit: and here's the original file, back mostly before I touched it at all:
[https://github.com/LibreShift/red-
moon/blob/ed2ec4fd1c68611d...](https://github.com/LibreShift/red-
moon/blob/ed2ec4fd1c68611d851a55f3ade141e6738e394c/app/src/main/java/com/jmstudios/redmoon/presenter/ScreenFilterPresenter.java)

------
osrec
Looking at lines 460-521 in the modified file
([https://github.com/miguelxpn/php-
src/blob/f4b2089b642d504be3...](https://github.com/miguelxpn/php-
src/blob/f4b2089b642d504be358b06bbae81fa688d8bf0e/ext/standard/http_fopen_wrapper.c#L460)),
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
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/3d9c02364db62a6d8e2794...](https://github.com/php/php-
src/commit/3d9c02364db62a6d8e27947ffe47dbfaad644efe#diff-e0dff85f21e939e4e2a778bddb8a72d7)

~~~
osrec
Ah, much cleaner!

------
Lazare
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
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.

------
kazinator

      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])

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

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

~~~
homero
Headers are just text mixed with body and hard to figure out where they stop

~~~
speps
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....](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.1)

------
wolco
That ends suddenly.

