# How to deal with bugs in vendored libraries? - Thread: https://externals.io/message/119502 - Original archive: https://news-web.php.net/php.internals/119502 - Started by Max Kellermann on 2023-02-09T10:19:02+00:00 - Messages: 9 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #119502 — Max Kellermann — 2023-02-09T10:19:02+00:00 URL: https://externals.io/message/119502#119502 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 --- ## #119503 — Rowan Tommins — 2023-02-09T12:37:52+00:00 URL: https://externals.io/message/119502#119503 · In reply to: #119502 > 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] --- ## #119504 — David CARLIER — 2023-02-09T12:51:35+00:00 URL: https://externals.io/message/119502#119504 · In reply to: #119503 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. --- ## #119505 — Max Kellermann — 2023-02-09T13:14:05+00:00 URL: https://externals.io/message/119502#119505 · In reply to: #119503 > 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 . > 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 --- ## #119508 — Rowan Tommins — 2023-02-09T15:29:59+00:00 URL: https://externals.io/message/119502#119508 · In reply to: #119505 > 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. > > > […] > of > > […] > precisely: > > […] > > 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 . > 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 > > […] > > 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] --- ## #119509 — Max Kellermann — 2023-02-09T16:05:30+00:00 URL: https://externals.io/message/119502#119509 · In reply to: #119508 > 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 --- ## #119510 — Kamil Tekiela — 2023-02-09T16:18:06+00:00 URL: https://externals.io/message/119502#119510 · In reply to: #119509 Hi Max, > - 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 --- ## #119514 — Rowan Tommins — 2023-02-09T16:57:51+00:00 URL: https://externals.io/message/119502#119514 · In reply to: #119509 > > […] > > 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] --- ## #119517 — Jakub Zelenka — 2023-02-09T21:18:08+00:00 URL: https://externals.io/message/119502#119517 · In reply to: #119502 Hi, > 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