Re: error_log binary unsafe

On Mon, Oct 28, 2013 at 1:03 PM, Joe Watkins <krakjoe@php.net> wrote: > On 10/28/2013 11:45 AM, Joe Watkins wrote: > >> On 10/28/2013 11:42 AM, Julien Pauli wrote: >> >>> On Mon, Oct 28, 2013 at 12:11 PM, Joe Watkins <krakjoe@php.net> wrote: >>> >>> On 10/28/2013 10:50 AM, Julien Pauli wrote: >>>> >>>> On Sun, Oct 27, 2013 at 7:14 AM, Joe Watkins <pthreads@pthreads.org> >>>>> wrote: >>>>> >>>>> On 10/26/2013 11:54 PM, Yasuo Ohgaki wrote: >>>>> >>>>>> >>>>>> Hi Joe, >>>>>> >>>>>>> >>>>>>> On Sun, Oct 27, 2013 at 7:48 AM, Yasuo Ohgaki <yohgaki@ohgaki.net >>>>>>> <mailto: >>>>>>> yohgaki@ohgaki.net>> wrote: >>>>>>> >>>>>>> On Sat, Oct 26, 2013 at 2:38 PM, Joe Watkins >>>>>>> >>>>>>> <pthreads@pthreads.org >>>>>>> <mailto:pthreads@pthreads.org>******> >>>>>>> wrote: >>>>>>> >>>>>>> Mail is not yet handled, TCP/IP is not supported any more, >>>>>>> streams are binary safe. >>>>>>> The SAPI and default error logging mechanism are all that >>>>>>> require attention. >>>>>>> >>>>>>> The patch is not final and doesn't include a fix for >>>>>>> every >>>>>>> implementation of SAPI. >>>>>>> >>>>>>> I don't see the need for confusion ?? >>>>>>> >>>>>>> >>>>>>> Generally speaking, I'm not against making >>>>>>> functions/features >>>>>>> binary safe. >>>>>>> >>>>>>> There are many implementations of syslog/SAPI and not sure >>>>>>> if it >>>>>>> is good for all. >>>>>>> It could cause BC issue also. For example, application like >>>>>>> OSSEC >>>>>>> HIDS detects >>>>>>> possible intrusion by analyzing logs. Patching SAPI may break >>>>>>> these applications. >>>>>>> >>>>>>> I'm not against applying your patch to master, but it's >>>>>>> not for >>>>>>> released versions. >>>>>>> We needs UPGRADE note if the patch is applied. >>>>>>> >>>>>>> >>>>>>> I think it's good for master. >>>>>>> Do you have commit karma? >>>>>>> If not, I'm willing to merge your patch unless there are not >>>>>>> objections. >>>>>>> >>>>>>> Regards, >>>>>>> >>>>>>> -- >>>>>>> Yasuo Ohgaki >>>>>>> yohgaki@ohgaki.net >>>>>>> <mailto:yohgaki@ohgaki.net> >>>>>>> >>>>>>> >>>>>>> I don't have karma ... >>>>>> >>>>>> There are lots of SAPI's, but this patch wasn't meant to >>>>>> implement >>>>>> them >>>>>> all, only to provide a route whereby they can implement binary safe >>>>>> log_message in the shape of log_message_ex. >>>>>> >>>>>> The patch implements binsafe log for cli and cgi, do we need to >>>>>> implement >>>>>> any more ?? >>>>>> >>>>>> >>>>>> Joe: Why do you use strlen() ? This leads to the same not binary safe >>>>> string, am I wrong ?? >>>>> >>>>> https://github.com/krakjoe/****php-src/commit/**<https://github.com/krakjoe/**php-src/commit/**> >>>>> be5f38ddd449c20230c042aef9757e****fb2ee08188#diff-** >>>>> >>>>> 1a9cfc6173e3a434387996e46086da****56R610<https://github.com/** >>>>> krakjoe/php-src/commit/**be5f38ddd449c20230c042aef9757e** >>>>> >>>>> fb2ee08188#diff-**1a9cfc6173e3a434387996e46086da**56R610<https://github.com/krakjoe/php-src/commit/be5f38ddd449c20230c042aef9757efb2ee08188#diff-1a9cfc6173e3a434387996e46086da56R610> >>>>> > >>>>> >>>>> >>>>> Julien Pauli >>>>> >>>>> >>>>> (sorry if you got this twice, my interweb is playing up) >>>> >>>> Hi Julien, >>>> >>>> The binary safe logging interface for SAPI is log_message_ex >>>> and >>>> the binary safe logging function for php is php_log_err_ex >>>> The old function must remain, and use string length just as >>>> it did >>>> before. >>>> >>>> >>> I see, that means that the actual patch does not turn logging to binary >>> safe logs, but gives functions for that. >>> Turning logs to binary safe would then mean tracking every unsafe log >>> function (php_log_err()) and turn it to php_log_err_ex() with an explicit >>> string length. >>> >>> >>> Julien Pauli >>> >>> >> >> http://lxr.php.net/search?q=&**defs=&refs=php_log_err&path=&** >> >> hist=&project=PHP_5_5<http://lxr.php.net/search?q=&defs=&refs=php_log_err&path=&hist=&project=PHP_5_5> >> >> >> Not the enormous task you imagine it to be :) >> >> Cheers >> Joe >> > > Actually, looking at the one occurrence of php_err_log in mysqlnd, it > doesn't need updating because it's not being used, the call to > mysqlnd_get_backtrace is wrong ... > > When it is in use, the backtrace provides a length and so we can switch to > php_err_log_ex easily. > > So, there isn't anything else to change, or not neceesarily ... > > What there is to consider is Apache and FPM: > > Apaches logging functions don't accept a length because they are > formatters. > I'm reluctant to touch apache logging at all, it could be a > massive BC break. > APR has functions for logging data that do accept a length and > would be binary safe, however, I've never used them, we want an apache geek > really to look at that ... > Knowing little bit of APR, this is "just" a library abstracting the OS. If there are apr function for binary safe logging, I guess they won't require a mass effort to implement, there shouldn't need an Apache guru IMO. The only thing to check, is the APR version they appeared, and if we are compatible with that. > > FPM has a custom logging interface (zlog) that doesn't accept a > length either, I've not looked too deeply into FPM, I'm not sure, do we > have an "fpm" expert to turn to, it never really comes up ?? > > Other SAPI's: > Theese should be updated to use binary safe logging > (log_message_ex), I've looked into the most popular, updated the cli and > cgi versions for reference. > > Oh an the mail function, php_mail, also doesn't accept a length ... > > Not sure where to go from here ... Not sure as well, if we can't turn the logging system to a full binary safe system, with no BC , then we shouldn't bother patching the 5.x branch and think about a deeper solution against master, which would then require an RFC. Julien.Pauli