Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96447
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
Date: Wed, 19 Oct 2016 06:41:06 +0900
Message-ID: <CAGa2bXbUmRZ8EK4HB=WyKLK5EcACXZ_wisH1v3=2ib81-HetUQ@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Bug fix and RFC/Merge rule
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi all,

Following commit is reverted because someone requested "RFC" and/or
demand "Merge approval from RM",

http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8

I don't understand the reasoning why it requires RFC nor Merge
approval. As we all know, there are many bug fixes without RFC nor
discussion.

So let's discuss about what bug fix requires RFC and merge approval.

The patch fixes "uniqid() is not unique enough" bug.
---------------
==Rationale==
uniqid() is simply _broken_ because it does not provide expected uniqueness due
to timestamp based php_combined_lcg().
(large warning is added to the manual recently)

unique id (time stamp) + entropy (timestamp based entropy)

Problems of this implementation
  - NTP is used to adjust system time.
  - Result is not reasonably unique by logic.

==Resolution==
  Replace php_combined_lcg() (Poor PRNG) to php_random_bytes() (CSPRNG).

  Patch
  http://git.php.net/?p=php-src.git;a=commitdiff;h=48f1a17886d874dc90867c669481804de90509e8
  (less than 30 lines of diff)

==BC impact==
  There is no BC.  php_random_bytes() raises exception for bad CSPRNG,
  but this very unlikely and such system shouldn't be used anyway.
---------------

Discussions in previous thread
---------------
=="new uniqid() can cause /dev/urandom cannot read error"==

This is FUD. Almost all "/dev/urandom cannot read" issues on the net
is due to "open_basedir" restriction. "open_basedir" is nothing to do
with internal functions, so the discussion is FUD.

=="Any new error is BC so the patch should be reverted"==

We do have many bug fixes emit new errors. We don't revert them at all
even when it's very common and annoying.

Example:  https://bugs.php.net/bug.php?id=73238
This bug fix made WordPress display few additional E_WARNING errors
that can be remove by php.ini or code fix.

=="Someone explicitly requested RFC/Merge approval, so it should be reverted"==

IMHO, the patch is very simple patch only fixes problem. Many bug
fixes include more severe BC issues than the patch, behavior changes
and raised errors. Yet, no RFC nor discussions.

If someone requested RFC/Merge request for very simple patch, should
we follow always?

=="It does not have to hurry"==

I agree with this. However, reverting simple bug fix patch does not make sense.
--------------

Question is:
  - What kind of bug fix requires RFC?
  - What kind of bug fix requires discussion and approval to merge
released versions?

It does not have to be generic. It's okay for providing reasons why
this specific bug fix requires RFC or merge approval. If this bug fix
requires RFC or merge approval, almost all bug fixes requires RFC and
merge approval.

Thank you.



P.S.
The original discussion is done in "[RFC][DISCUSSION] Improve uniqid()
uniqueness" thread.

After all, my question is "Should we discuss all bugs before commits?"
"The revert is valid and reasonable?"


--
Yasuo Ohgaki
yohgaki@ohgaki.net