error_log binary unsafe View as Markdown

Hi Joe,

     https://bugs.php.net/bug.php?**id=64439<https://bugs.php.net/bug.php?id=64439>

     What do we think of the following approach:

     https://github.com/php/php-**src/pull/505<https://github.com/php/php-src/pull/505>

Like laruence said, error logs are written to various places.
Handling only by SAPI will not solve issue, but introduces more
confusion. (i.e. double escape/conversion)

I also updated manual page that logs are written to various places.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

4713 case 1: /*send an email */
4714 if (!php_mail(opt, "PHP error_log message", message,
headers, NULL TSRMLS_CC)) {
4715 return FAILURE;
4716 }
4717 break;
4718
4719 case 2: /*send to an address */
4720 php_error_docref(NULL TSRMLS_CC, E_WARNING, "TCP/IP
option not available!");
4721 return FAILURE;
4722 break;
4723
4724 case 3: /*save to a file /
4725 stream = php_stream_open_wrapper(opt, "a",
IGNORE_URL_WIN | REPORT_ERRORS, NULL);
4726 if (!stream) {
4727 return FAILURE;
4728 }
4729 php_stream_write(stream, message, message_len);
4730 php_stream_close(stream);
4731 break;
4732
4733 case 4: / send to SAPI */
4734 if (sapi_module.log_message) {
4735 sapi_module.log_message(message TSRMLS_CC);
4736 } else {
4737 return FAILURE;
4738 }
4739 break;
4740
4741 default:
4742 php_log_err(message TSRMLS_CC);
4743 break;

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 ??

I don't see the need to ignore the problem either.

Cheers
Joe

The patch implements binsafe log for cli and cgi, do we need to implement
any more ??

It's better to check & fix all SAPIs :)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Indeed ...

But the original question I asked was for approval on the approach ...

I guess I got that ??

I don't mind implementing other SAPI's at all, I was just wondering if
the approach is satisfactory ...

Cheers
Joe

On Sun, Oct 27, 2013 at 7:14 AM, Joe Watkins pthreads@pthreads.org
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/be5f38ddd449c20230c042aef9757efb2ee08188#diff-1a9cfc6173e3a434387996e46086da56R610

If I'm reading the patch correctly, that should be resolved by updating all
php_log_err() references in the rest of the project to use
php_log_err_ex() instead. I'm not sure if that was deliberately left out,
though.

Julien Pauli

--

Tjerk

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

Cheers
Joe