Everything

php

How to deal with bugs in vendored libraries?

php.internals

Max Kellermann

3 years ago

Hi,

what happens if there is a bug in a vendored library, but upstream
refuses to fix it?

Last month, my PR for removing obsolete C99 integer checks was merged:

https://github.com/php/php-src/pull/10304

This change speeds up configure and removes code that violates the C99
spec.

It included a fix for ext/date/lib/ which I then learned is a vendored
library; Christoph M. Becker asked me to submit the same fix upstream,
which I did:

https://github.com/derickr/timelib/pull/141

As you can see, the PR was closed with a comment that did not explain
why.  After it was pointed out to them that the existing code violates
the C99 standard, they locked the PR, preventing further discussion.

Not only that; they also reverted my fix in php-src, reintroducing the
C99 spec violation:

https://github.com/php/php-src/commit/0df92d218e88a0070fcebd5391e7

... even though UPGRADING.INTERNALS contains an explanation that these
obsolete feature macros are no longer present in PHP's config headers
and should not be used.

Two questions remain for me:

- Should somebody with push access to php-src silently/secretly,
without any discussion, revert such a fix?  The revert seems to have
been directly pushed to php-src without a PR and with no chance for
others to review it.

- In general, if a vendored library has a bug (like the C99 spec
violation, or any other bug) that the upstream refuses to fix or
even discuss, shall the copy in php-src be patched?  Are deviations
from vanilla upstream possible?  Of course, this causes extra work
for syncing with new upstream released, but tools like quilt could
help with that.

Max

Rowan Collins

3 years ago

On Thu, 9 Feb 2023 at 10:19, Max Kellermann <max+php@blarg.de> wrote:

> Hi,
>
> what happens if there is a bug in a vendored library, but upstream
> refuses to fix it?
>

I don't have an answer to the procedural question, or the specific
technical issue, but I would like to say three things:

Firstly, let's try to keep this discussion civil, and assume good faith on
both sides. Parts of your e-mail read like accusations of bad behaviour,
rather than genuinely trying to understand what happened, and how we can
collectively avoid it happening in future.

Secondly, note that Derick Rethans is the maintainer of both timelib and
the ext/date extension in php-src. So while we can discuss the hypothetical
question of how to handle a disagreement between php-src and upstream
library maintainers, it wouldn't apply in this case anyway, because it
would require Derick to disagree with himself.

Thirdly, it's not clear to me which of the following statements is true of
this change, and it might help the conversation to clarify more precisely:
a) The code you removed *violates* the C99 spec?
b) The code you removed is *guaranteed to be pointless* under the C99 spec
(but does not violate it)?
c) The code you removed is *pointless in this particular case* because of a
combination of the C99 spec and other factors (but might be reasonable in
other circumstances)?

Regards,

--
Rowan Tommins
[IMSoP]

David CARLIER

3 years ago

Hi,

On Thu, 9 Feb 2023 at 12:38, Rowan Tommins <rowan.collins@gmail.com> wrote:

> On Thu, 9 Feb 2023 at 10:19, Max Kellermann <max+php@blarg.de> wrote:
>
> > Hi,
> >
>
>
> Firstly, let's try to keep this discussion civil, and assume good faith on
> both sides.
>
>
>  That a kind of constant with you, Max, through your PR too.

If you could be more diplomatic and improve on that, that would be awesome.

David C.

Cheers.

Max Kellermann

3 years ago

On 2023/02/09 13:37, Rowan Tommins <rowan.collins@gmail.com> wrote:

> Firstly, let's try to keep this discussion civil, and assume good faith on
> both sides. Parts of your e-mail read like accusations of bad behaviour,
> rather than genuinely trying to understand what happened, and how we can
> collectively avoid it happening in future.

While I do have an opinion on whether I consider Derick Rethan's
behavior bad (yes, I do), that's not the point here.  I don't know how
to proceed after my PR thread was locked - that's an unequivocal sign
of refusal to discuss the issue.  The issue still exists, and I'm here
for your advice on how to resolve this.  I'm desperate.

> Secondly, note that Derick Rethans is the maintainer of both timelib and
> the ext/date extension in php-src. So while we can discuss the hypothetical
> question of how to handle a disagreement between php-src and upstream
> library maintainers, it wouldn't apply in this case anyway, because it
> would require Derick to disagree with himself.

That depends.  Did the PHP project decide to go C99 starting with
version 8?  What does that mean for maintainers - can they decide to
make code changes that are not compliant with that decision?

That is a honest question.  I don't know how PHP works.

That's why I asked whether "secret" reverts without discussion are
considered good behavior.  Maybe you believe maintainers should do
that - but that would be surprising for me.

> Thirdly, it's not clear to me which of the following statements is true of
> this change, and it might help the conversation to clarify more precisely:
> a) The code you removed *violates* the C99 spec?

This.  The code in question declares typedefs that are reserved words
in the C99 spec section 7.26.8; not just reserved, they conflict with
actual typedefs from <stdint.h>.

> b) The code you removed is *guaranteed to be pointless* under the C99 spec
> (but does not violate it)?

No.  It is not pointless.  Those declarations occupy reserved words,
and that is not allowed.

> c) The code you removed is *pointless in this particular case* because of a
> combination of the C99 spec and other factors (but might be reasonable in
> other circumstances)?

I don't understand this one, but it doesn't sound like it applies.

Max

Rowan Collins

3 years ago

On Thu, 9 Feb 2023 at 13:14, Max Kellermann <max+php@blarg.de> wrote:

> The issue still exists, and I'm here
> for your advice on how to resolve this.  I'm desperate.
>

Is this a critical security issue? If not, there's no need to be desperate;
just take a breath, explain what you were trying to achieve, and be
genuinely open to discussion and suggestions. In your head, I'm sure it's
very clear what you're working on, and why it's the right thing to do; but
clearly, it doesn't seem as obvious to everyone involved.

> That's why I asked whether "secret" reverts without discussion are
> considered good behavior.  Maybe you believe maintainers should do
> that - but that would be surprising for me.
>

This is where I'm suggesting you assume good faith: what looks like a
"secret revert" probably feels like something entirely different to Derick.

>
> > Thirdly, it's not clear to me which of the following statements is true
> of
> > this change, and it might help the conversation to clarify more
> precisely:
> > a) The code you removed *violates* the C99 spec?
>
> This.  The code in question declares typedefs that are reserved words
> in the C99 spec section 7.26.8; not just reserved, they conflict with
> actual typedefs from <stdint.h>.
>

OK, that seems clear. As far as I can see, this is the first time on this
thread or either of the PR threads that you've actually explained that
violation.

> b) The code you removed is *guaranteed to be pointless* under the C99 spec
> > (but does not violate it)?
>
> No.  It is not pointless.  Those declarations occupy reserved words,
> and that is not allowed.
>

OK, so follow-up question: what gives you confidence that the change is
*safe*? Sometimes, technical violations of a spec are necessary for the
practical realities of the situation. I think you've answered this question
in the PR thread, but I'm trying to get the explanation all in one place,
because you've mentioned different details at different times.

Regards,

--
Rowan Tommins
[IMSoP]

Max Kellermann

3 years ago

On 2023/02/09 16:29, Rowan Tommins <rowan.collins@gmail.com> wrote:

> This is where I'm suggesting you assume good faith: what looks like a
> "secret revert" probably feels like something entirely different to Derick.

Timeline:

- Jan 13 11:34 AM: PR https://github.com/derickr/timelib/pull/141/files

- Jan 18 4:34 PM: PR was locked

- Jan 19 7:49 PM: commit
https://github.com/php/php-src/commit/0df92d218e88a0 pushed to
php-src

Look how the commit is exactly a revert of the timelib PR that Derick
Rethans closed and locked just the day before.

I'm open to hearing Derick's side of the story.

> OK, that seems clear. As far as I can see, this is the first time on this
> thread or either of the PR threads that you've actually explained that
> violation.

I explained it already:

https://github.com/derickr/timelib/pull/141#issuecomment-1386800720

> OK, so follow-up question: what gives you confidence that the change is
> *safe*? Sometimes, technical violations of a spec are necessary for the
> practical realities of the situation.

This piece of code was written when these types were not yet in the C
standard (the C standard used by PHP at the time); but the code became
wrong as soon as PHP switched to C99.

So it's not like somebody decided "we need to violate the spec for
some practical reality"; that spec didn't apply at the time.

What my code change does is adjust PHP to the newly adopted C
standard; this should have been part of PHP's C99 transition, but was
probably forgotten.

Max

Kamil Tekiela

3 years ago

Hi Max,

On Thu, 9 Feb 2023 at 16:05, Max Kellermann <max+php@blarg.de> wrote:

> - Jan 19 7:49 PM: commit
>   https://github.com/php/php-src/commit/0df92d218e88a0 pushed to
>   php-src
>
> Look how the commit is exactly a revert of the timelib PR that Derick
> Rethans closed and locked just the day before.
>

Ahh so it seems to be a big misunderstanding. Derrick didn't undo your
commit. Derrick simply synched timelib. The file in the commit belongs to
timelib not PHP.

Derrick is the author and the maintainer of that library. As the author, he
decides on what is merged into timelib. You submitted a PR that was
declined. No big deal. But the file should not be modified in PHP because
PHP has a readonly copy of that library. It will be overwritten whenever
Derrick syncs it up.

I'm not sure what impact this has on PHP, but as long as it compiles fine
and there are no bugs, I see no reason to pressure the maintainer of
timelib library to change their code. If it meant that the current release
of timelib suddenly becomes inoperable with PHP then we would have an
issue.

Regards,
Kamil

Rowan Collins

3 years ago

On Thu, 9 Feb 2023 at 16:05, Max Kellermann <max+php@blarg.de> wrote:

> > OK, that seems clear. As far as I can see, this is the first time on this
> > thread or either of the PR threads that you've actually explained that
> > violation.
>
> I explained it already:
>
>  https://github.com/derickr/timelib/pull/141#issuecomment-1386800720
>

Right, I missed that amongst the other comments. Which, again, is why I was
prompting you to clearly summarise the rationale for the change in one
place, to clear up any misunderstandings.

Regards,

--
Rowan Tommins
[IMSoP]

Jakub Zelenka

3 years ago

Hi,

On Thu, Feb 9, 2023 at 10:19 AM Max Kellermann <max+php@blarg.de> wrote:

> Hi,
>
> what happens if there is a bug in a vendored library, but upstream
> refuses to fix it?
>
> Last month, my PR for removing obsolete C99 integer checks was merged:
>
>  https://github.com/php/php-src/pull/10304
>
> This change speeds up configure and removes code that violates the C99
> spec.
>
> It included a fix for ext/date/lib/ which I then learned is a vendored
> library; Christoph M. Becker asked me to submit the same fix upstream,
> which I did:
>
>  https://github.com/derickr/timelib/pull/141
>
> As you can see, the PR was closed with a comment that did not explain
> why.  After it was pointed out to them that the existing code violates
> the C99 standard, they locked the PR, preventing further discussion.
>
> Not only that; they also reverted my fix in php-src, reintroducing the
> C99 spec violation:
>
>  https://github.com/php/php-src/commit/0df92d218e88a0070fcebd5391e7
>
> ... even though UPGRADING.INTERNALS contains an explanation that these
> obsolete feature macros are no longer present in PHP's config headers
> and should not be used.
>
> Two questions remain for me:
>
> - Should somebody with push access to php-src silently/secretly,
>   without any discussion, revert such a fix?  The revert seems to have
>   been directly pushed to php-src without a PR and with no chance for
>   others to review it.
>
>

Yes if it's a maintainer of the code which is the case here. Historically
we didn't use PR's for normal fixes. We started doing so mainly to run
tests rather than getting feedback. That said I think it's good to do PR's
for most changes as it's safer and feedback is good but it has never been a
requirement.

> - In general, if a vendored library has a bug (like the C99 spec
>   violation, or any other bug) that the upstream refuses to fix or
>   even discuss, shall the copy in php-src be patched?  Are deviations
>   from vanilla upstream possible?  Of course, this causes extra work
>   for syncing with new upstream released, but tools like quilt could
>   help with that.
>
>

I don't think we would want to do this in general unless it's something
critical or security related which this is certainly not.

The only thing that you can do here is to create an RFC but I'm pretty sure
that it would be just waste of time as it would be destined to fail. So the
best thing that you can do is to just forget about it and move on otherwise
I think you are wasting your time...

Regards

Jakub