r82112 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r82111‎ | r82112 | r82113 >
Date:12:21, 14 February 2011
Author:tstarling
Status:deferred (Comments)
Tags:
Comment:
* Use debug_print_backtrace() to make the backtrace, instead of print_r(). The output is not pretty, but definitely better than it was. Uses output buffers, like several examples in basic_functions.c.
* Added a README file documenting the php.ini settings
* Renamed logging_file to log_file
* Renamed concise_backtrace_in_error_log to backtrace_in_php_error_message. There were two error logs and it wasn't really clear which one was meant. The fact that it's concise is documented in the README file now, so we don't need it in the name.
* Stop using incomprehensible integer literals for log_level. Introduce wmerrors.log_backtrace instead. If you don't want a log file at all, you can set wmerrors.log_file to an empty string.
* Don't use TSRMLS_FETCH() unless necessary. Use TSRMLS_DC in wmerrors_get_backtrace() instead.
* Renamed wmerrors_get_backtrace() to wmerrors_get_concise_backtrace() so that it doesn't get confused with the verbose backtrace
* Suppress compiler warning due to const char confusion in call to php_stream_open_wrapper()
* Provide human-readable descriptions of the error types, copied from main.c.
* Use local time in the error log, not UTC, for consistency with PHP.
Modified paths:
  • /trunk/php/wmerrors/README (added) (history)
  • /trunk/php/wmerrors/php_wmerrors.h (modified) (history)
  • /trunk/php/wmerrors/wmerrors.c (modified) (history)

Diff [purge]

Index: trunk/php/wmerrors/README
——@@ -0,0 +1,64 @@
 2+This is an extension for enhancing and customising the handling of PHP errors.
 3+
 4+Compile and install it in the usual way, there are no special dependencies:
 5+
 6+ phpize
 7+ ./configure
 8+ make
 9+ make install
 10+
 11+The options are:
 12+
 13+wmerrors.enabled
 14+
 15+ Set this to true to enable custom error handling. This allows the extension
 16+ to be disabled with ini_set() at any time before the fatal error is
 17+ generated.
 18+
 19+wmerrors.message_file
 20+
 21+ Set this to the path to an HTML file which you wish to be displayed to your
 22+ users. A sample HTML file is distributed with this extension in error.html.
 23+
 24+ If this is set, display_errors should be disabled to prevent a duplicate
 25+ message from being shown.
 26+
 27+ The error HTML may contain the following special variables, which will be
 28+ replaced with the appropriate values when the error message is generated.
 29+ Unlike PHP's display_errors, these values will be properly escaped to avoid
 30+ XSS:
 31+
 32+ - $file: The filename in which the error occurred
 33+ - $line: The line number at which the error occurred
 34+ - $message: The error message
 35+
 36+wmerrors.log_file
 37+
 38+ The name of a file to send error reports to. This is similar to PHP's
 39+ error_log, except that it provides several additional features. Logging to
 40+ a TCP or UDP network socket is supported, using a URL of the form:
 41+
 42+ udp://<host>:<port>
 43+ tcp://<host>:<port>
 44+
 45+ For example:
 46+
 47+ udp://logger:8420
 48+
 49+wmerrors.log_backtrace
 50+
 51+ Set this to true if you want the error report sent to wmerrors.log_file to
 52+ include a backtrace.
 53+
 54+wmerrors.ignore_logging_errors
 55+
 56+ If an error is encountered while opening or writing to the file specified
 57+ in wmerrors.log_file, then the error will be ignored if this is set to
 58+ true. If it is set to false, the error will be handled with PHP's standard
 59+ error handling, and so will be available via display_errors or error_log.
 60+
 61+wmerrors.backtrace_in_php_error_message
 62+
 63+ If this is true, a concise backtrace, listing base filenames (not including
 64+ the path) and line numbers only, will be included in the error message
 65+ which is passed through to PHP, for output into error_log.
Index: trunk/php/wmerrors/php_wmerrors.h
——@@ -25,13 +25,12 @@
2626
2727 ZEND_BEGIN_MODULE_GLOBALS(wmerrors)
2828 char * message_file;
29 - char * logging_file;
 29+ char * log_file;
3030 int recursion_guard;
3131 int enabled;
32 - long int log_level;
 32+ int log_backtrace;
3333 int ignore_logging_errors;
34 - int concise_backtrace_in_error_log;
35 - smart_str log_buffer;
 34+ int backtrace_in_php_error_message;
3635 ZEND_END_MODULE_GLOBALS(wmerrors)
3736
3837
Index: trunk/php/wmerrors/wmerrors.c
——@@ -17,9 +17,9 @@
1818
1919 void wmerrors_cb(int type, const char *error_filename, const uint error_lineno, const char *format, va_list args);
2020 static void wmerrors_show_message(int type, const char *error_filename, const uint error_lineno, const char *format, va_list args TSRMLS_DC);
21 -void wmerrors_get_backtrace(smart_str *s);
 21+static void wmerrors_get_concise_backtrace(smart_str *s TSRMLS_DC);
 22+static void write_full_backtrace(php_stream *logfile_stream);
2223
23 -
2424 ZEND_DECLARE_MODULE_GLOBALS(wmerrors)
2525
2626 zend_function_entry wmerrors_functions[] = {
——@@ -52,10 +52,10 @@
5353 PHP_INI_BEGIN()
5454 STD_PHP_INI_BOOLEAN("wmerrors.enabled", "0", PHP_INI_ALL, OnUpdateBool, enabled, zend_wmerrors_globals, wmerrors_globals )
5555 STD_PHP_INI_ENTRY("wmerrors.message_file", "", PHP_INI_ALL, OnUpdateString, message_file, zend_wmerrors_globals, wmerrors_globals)
56 - STD_PHP_INI_ENTRY("wmerrors.logging_file", "", PHP_INI_ALL, OnUpdateString, logging_file, zend_wmerrors_globals, wmerrors_globals)
57 - STD_PHP_INI_ENTRY("wmerrors.log_level", "0", PHP_INI_ALL, OnUpdateLong, log_level, zend_wmerrors_globals, wmerrors_globals)
 56+ STD_PHP_INI_ENTRY("wmerrors.log_file", "", PHP_INI_ALL, OnUpdateString, log_file, zend_wmerrors_globals, wmerrors_globals)
 57+ STD_PHP_INI_BOOLEAN("wmerrors.log_backtrace", "0", PHP_INI_ALL, OnUpdateBool, log_backtrace, zend_wmerrors_globals, wmerrors_globals)
5858 STD_PHP_INI_BOOLEAN("wmerrors.ignore_logging_errors", "0", PHP_INI_ALL, OnUpdateBool, ignore_logging_errors, zend_wmerrors_globals, wmerrors_globals)
59 - STD_PHP_INI_BOOLEAN("wmerrors.concise_backtrace_in_error_log", "0", PHP_INI_ALL, OnUpdateBool, concise_backtrace_in_error_log, zend_wmerrors_globals, wmerrors_globals)
 59+ STD_PHP_INI_BOOLEAN("wmerrors.backtrace_in_php_error_message", "0", PHP_INI_ALL, OnUpdateBool, backtrace_in_php_error_message, zend_wmerrors_globals, wmerrors_globals)
6060 PHP_INI_END()
6161
6262 void (*old_error_cb)(int type, const char *error_filename, const uint error_lineno, const char *format, va_list args);
——@@ -63,8 +63,8 @@
6464 static void php_wmerrors_init_globals(zend_wmerrors_globals *wmerrors_globals)
6565 {
6666 wmerrors_globals->message_file = NULL;
67 - wmerrors_globals->logging_file = NULL;
68 - wmerrors_globals->log_level = 0;
 67+ wmerrors_globals->log_file = NULL;
 68+ wmerrors_globals->log_backtrace = 0;
6969 }
7070
7171 PHP_MINIT_FUNCTION(wmerrors)
——@@ -89,7 +89,6 @@
9090 PHP_RINIT_FUNCTION(wmerrors)
9191 {
9292 WMERRORS_G(recursion_guard) = 0;
93 - WMERRORS_G(log_buffer).c = NULL;
9493 return SUCCESS;
9594 }
9695
——@@ -152,14 +151,14 @@
153152 wmerrors_show_message(type, error_filename, error_lineno, format, args TSRMLS_CC);
154153 }
155154
156 - if ( WMERRORS_G(enabled) && WMERRORS_G(log_level) ) {
 155+ if ( WMERRORS_G(enabled) ) {
157156 /* Log the error */
158157 wmerrors_log_error(type, error_filename, error_lineno, format, args TSRMLS_CC);
159158 }
160159
161160 /* Put a concise backtrace in the normal output */
162 - if (WMERRORS_G(concise_backtrace_in_error_log))
163 - wmerrors_get_backtrace(&new_filename);
 161+ if (WMERRORS_G(backtrace_in_php_error_message))
 162+ wmerrors_get_concise_backtrace(&new_filename TSRMLS_CC);
164163 smart_str_appendl(&new_filename, error_filename, strlen(error_filename));
165164 smart_str_0(&new_filename);
166165
——@@ -172,14 +171,13 @@
173172 }
174173
175174 /* Obtain a concisely formatted backtrace */
176 -void wmerrors_get_backtrace(smart_str *s) {
 175+static void wmerrors_get_concise_backtrace(smart_str *s TSRMLS_DC) {
177176 zval *trace, **entry, **file, **line, *line_copy;
178177 HashPosition pos;
179178 char *basename;
180179 size_t basename_len;
181180 int use_copy;
182181
183 - TSRMLS_FETCH();
184182 ALLOC_INIT_ZVAL(trace);
185183 zend_fetch_debug_backtrace(trace, 0, 0 TSRMLS_CC);
186184
——@@ -217,7 +215,7 @@
218216 FREE_ZVAL(trace);
219217 }
220218
221 -static php_stream * open_logging_file(const char* stream_name) {
 219+static php_stream * open_log_file(const char* stream_name) {
222220 php_stream * stream;
223221 int err; char *errstr = NULL;
224222 struct timeval tv;
——@@ -228,7 +226,7 @@
229227
230228 if ( strncmp( stream_name, "tcp://", 6 ) && strncmp( stream_name, "udp://", 6 ) ) {
231229 /* Is it a wrapper? */
232 - stream = php_stream_open_wrapper(stream_name, "ab", flags, NULL);
 230+ stream = php_stream_open_wrapper((char*)stream_name, "ab", flags, NULL);
233231 } else {
234232 /* Maybe it's a transport? */
235233 double timeout = FG(default_socket_timeout);
——@@ -244,26 +242,19 @@
245243 return stream;
246244 }
247245
248 -/* Callback for zend_print_zval_r_ex()
249 - * Writes to the global buffer
250 - */
251 -static int wmerrors_write_trace(const char *str, uint str_length) {
252 - TSRMLS_FETCH();
253 - smart_str_appendl(&WMERRORS_G(log_buffer), str, str_length);
254 - return str_length;
255 -}
256 -
257246 static void wmerrors_log_error(int type, const char *error_filename, const uint error_lineno, const char *format, va_list args TSRMLS_DC) {
258 - char *tmp1; zval *trace; char *error_time_str;
259 - int tmp1_len; va_list my_args;
 247+ char *tmp1;
 248+ int tmp1_len;
 249+ char *error_time_str;
 250+ va_list my_args;
260251 php_stream *logfile_stream;
261252
262 - if ( !WMERRORS_G(enabled) || !WMERRORS_G(log_level) ) {
 253+ if ( !WMERRORS_G(enabled) ) {
263254 /* Redundant with the caller */
264255 return;
265256 }
266257
267 - if ( !WMERRORS_G(logging_file) || *WMERRORS_G(logging_file) == '\0') {
 258+ if ( !WMERRORS_G(log_file) || *WMERRORS_G(log_file) == '\0') {
268259 /* No log file configured */
269260 return;
270261 }
——@@ -271,7 +262,7 @@
272263 /* Try opening the logging file */
273264 /* Set recursion_guard==2 whenever we're doing something to the log file */
274265 WMERRORS_G(recursion_guard) = 2;
275 - logfile_stream = open_logging_file( WMERRORS_G(logging_file) );
 266+ logfile_stream = open_log_file( WMERRORS_G(log_file) );
276267 WMERRORS_G(recursion_guard) = 1;
277268 if ( !logfile_stream ) {
278269 return;
——@@ -282,28 +273,63 @@
283274 tmp1_len = vspprintf(&tmp1, 0, format, my_args);
284275 va_end(my_args);
285276
286 - /* Log the error (log_level >= 1) */
287 - error_time_str = php_format_date("d-M-Y H:i:s", 11, time(NULL), 0 TSRMLS_CC);
288 - php_stream_printf(logfile_stream TSRMLS_CC, "[%s UTC] %s: %.*s at %s on line %u%s", error_time_str, error_type_to_string(type), tmp1_len, tmp1, error_filename, error_lineno, PHP_EOL);
 277+ /* Log the error */
 278+ error_time_str = php_format_date("d-M-Y H:i:s", 11, time(NULL), 1 TSRMLS_CC);
 279+ php_stream_printf(logfile_stream TSRMLS_CC, "[%s] %s: %.*s at %s on line %u%s", error_time_str, error_type_to_string(type), tmp1_len, tmp1, error_filename, error_lineno, PHP_EOL);
289280 efree(error_time_str);
290281 efree(tmp1);
291282
292283 /* Write a backtrace */
293 - if ( WMERRORS_G(log_level) >= 2 ) {
294 - ALLOC_INIT_ZVAL(trace);
295 - zend_fetch_debug_backtrace(trace, 0, 0 TSRMLS_CC);
296 - zend_print_zval_r_ex(wmerrors_write_trace, trace, 4 TSRMLS_CC);
297 - FREE_ZVAL(trace);
298 -
299 - WMERRORS_G(recursion_guard) = 2;
300 - php_stream_write(logfile_stream, WMERRORS_G(log_buffer).c, WMERRORS_G(log_buffer).len TSRMLS_CC);
301 - WMERRORS_G(recursion_guard) = 1;
302 - smart_str_free( &WMERRORS_G(log_buffer) ); /* Free and reset the buffer */
 284+ if ( WMERRORS_G(log_backtrace) ) {
 285+ write_full_backtrace(logfile_stream TSRMLS_CC);
303286 }
304287
305288 php_stream_close( logfile_stream );
306289 }
307290
 291+
 292+/**
 293+ * Write a full backtrace to a stream
 294+ */
 295+void write_full_backtrace(php_stream *logfile_stream) {
 296+ zval *trace;
 297+ zend_fcall_info fci = empty_fcall_info;
 298+ zend_fcall_info_cache fcc = empty_fcall_info_cache;
 299+ zval *backtrace_retval, backtrace_fname;
 300+ int status;
 301+
 302+ /* Start an output buffer */
 303+ php_start_ob_buffer(NULL, 0, 1 TSRMLS_CC);
 304+
 305+ /* Call debug_print_backtrace */
 306+ ZVAL_STRING(&backtrace_fname, "debug_print_backtrace", 1);
 307+ status = zend_fcall_info_init(&backtrace_fname, IS_CALLABLE_STRICT, &fci, &fcc, NULL, NULL);
 308+ if (status != SUCCESS) {
 309+ zval_dtor(&backtrace_fname);
 310+ return;
 311+ }
 312+
 313+ fci.retval_ptr_ptr = &backtrace_retval;
 314+ zend_call_function(&fci, &fcc TSRMLS_CC);
 315+ if (backtrace_retval) {
 316+ zval_ptr_dtor(&backtrace_retval);
 317+ }
 318+ zval_dtor(&backtrace_fname);
 319+
 320+ /* Get the trace */
 321+ ALLOC_INIT_ZVAL(trace);
 322+ php_ob_get_buffer(trace TSRMLS_CC);
 323+ php_end_ob_buffer(0, 0 TSRMLS_CC);
 324+
 325+ /* Write it */
 326+ convert_to_string(trace);
 327+ WMERRORS_G(recursion_guard) = 2;
 328+ php_stream_write(logfile_stream, Z_STRVAL_P(trace), Z_STRLEN_P(trace) TSRMLS_CC);
 329+ WMERRORS_G(recursion_guard) = 1;
 330+
 331+ zval_ptr_dtor(&trace);
 332+}
 333+
308334 static void wmerrors_show_message(int type, const char *error_filename, const uint error_lineno, const char *format, va_list args TSRMLS_DC)
309335 {
310336 php_stream *stream;
——@@ -386,30 +412,34 @@
387413 }
388414
389415 static const char* error_type_to_string(int type) {
390 - int i;
391 - #define ErrorType(x) {x, #x}
392 - static struct { int type; const char* name; } error_names[] = {
393 - ErrorType(E_ERROR),
394 - ErrorType(E_CORE_ERROR),
395 - ErrorType(E_COMPILE_ERROR),
396 - ErrorType(E_USER_ERROR),
397 - ErrorType(E_RECOVERABLE_ERROR),
398 - ErrorType(E_WARNING),
399 - ErrorType(E_CORE_WARNING),
400 - ErrorType(E_COMPILE_WARNING),
401 - ErrorType(E_USER_WARNING),
402 - ErrorType(E_PARSE),
403 - ErrorType(E_NOTICE),
404 - ErrorType(E_USER_NOTICE),
405 - ErrorType(E_STRICT),
406 - ErrorType(E_DEPRECATED),
407 - ErrorType(E_USER_DEPRECATED)
408 - };
409 -
410 - for (i=0; i < sizeof(error_names)/sizeof(error_names[0]); i++) {
411 - if (type == error_names[i].type) {
412 - return error_names[i].name;
413 - }
 416+ /** Copied from php_error_cb() */
 417+ switch (type) {
 418+ case E_ERROR:
 419+ case E_CORE_ERROR:
 420+ case E_COMPILE_ERROR:
 421+ case E_USER_ERROR:
 422+ return "Fatal error";
 423+ case E_RECOVERABLE_ERROR:
 424+ return "Catchable fatal error";
 425+ case E_WARNING:
 426+ case E_CORE_WARNING:
 427+ case E_COMPILE_WARNING:
 428+ case E_USER_WARNING:
 429+ return "Warning";
 430+ break;
 431+ case E_PARSE:
 432+ return "Parse error";
 433+ case E_NOTICE:
 434+ case E_USER_NOTICE:
 435+ return "Notice";
 436+ case E_STRICT:
 437+ return "Strict Standards";
 438+ case E_DEPRECATED:
 439+ case E_USER_DEPRECATED:
 440+ return "Deprecated";
 441+ default:
 442+ return "Unknown error";
414443 }
415 - return "Unknown error";
416444 }
 445+
 446+

Follow-up revisions

RevisionCommit summaryAuthorDate
r82152Made wmerrors_cb() static....platonides22:55, 14 February 2011
r82208* Suppress warning from php_format_date() per CR r82112....tstarling02:40, 16 February 2011

Comments

#Comment by Platonides (talk | contribs)   22:40, 14 February 2011

I used UTC to avoid the timezone warning that you have reintroduced with this revision. Moreover, it says "PHP Warning: Unknown: It is not safe to rely on the system's timezone settings..." so something in the changed error_type_to_string() seems wrong.

I haven't reviewed wmerrors_get_concise_backtrace() or write_full_backtrace() yet but otherwise looks quite good.

#Comment by Tim Starling (talk | contribs)   22:47, 14 February 2011

The warning should be suppressed by the recursion guard. The code is the same in php_log_err(), and presumably the warning is suppressed in the same way. How are you seeing this warning?

#Comment by Platonides (talk | contribs)   15:08, 15 February 2011

I know, I also looked at it wondering what it did to disable such warning.

I'm just calling it with a non-existant funciton to trigger an error. 

$ php -r'foo();'
PHP Warning:  Unknown: It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'Europe/Berlin' for 'CET/1.0/no DST' instead in Command line code on line 1
PHP Fatal error:  Call to undefined function foo() in Command line code:1 Command line code on line 1

Whereas if wmerrors is disabled: 

$ php -r'foo();'
PHP Fatal error:  Call to undefined function foo() in Command line code on line 1
#Comment by Tim Starling (talk | contribs)   22:02, 15 February 2011

It shouldn't do it from within MediaWiki, we suppress that warning in Setup.php. You could just set date.timezone like it says.

#Comment by Platonides (talk | contribs)   23:44, 15 February 2011

Indeed (at least if the error doesn't appear before running Setup.php). But I thought we wanted wmerrors to be also usable by third parties.

#Comment by Tim Starling (talk | contribs)   02:42, 16 February 2011

Fixed in r82208.

Status & tagging log