0012410: Trailing content in plugin or core files destroys GraphViz graphs - MantisBT

ID	Project	Category	View Status	Date Submitted	Last Update
0012410	mantisbt	reports	public	2010-09-29 03:26	2016-07-21 15:23

Reporter	Renegade	Assigned To	jreese
Priority	normal	Severity	minor	Reproducibility	always
Status	closed	Resolution	won't fix
Product Version	1.2.3

Summary	0012410: Trailing content in plugin or core files destroys GraphViz graphs
Description	Short version: Any content after the closing PHP tag of any PHP file will be sent as raw text to the client. Since that is even the case for PHP files pretending to be images (such as graphs), any trailing content in included files will corrupt the file structure of the image and make it unreadable. This primarily affects graphs in combination with plugins, a simple patch is attached.
Steps To Reproduce	Open any given plugin. Find the ?> closing PHP tag. Append random characters. Save/install/load the plugin. Watch your graphs fail to load due to being corrupted.
Additional Information	Long version: The webserver interprets PHP files as text or HTML content to be served, after having been preprocessed by PHP. That is the reason why stuff like <body><?php echo 'Hello World!'; ?></body> works - anything outside PHP is just pushed as is. Due to Mantis's architecture, before a graph is created in bug_relationship_graph_img.php, pretty much the entire system is loaded - including the plugin API and all installed plugins. At this point, the same principle applies - if the primary plugin file contains non-PHP content, said content is sent as it is. However: Since in the case of bug_relationship_graph_img.php the file pretends to be a PNG or other image file instead, the content being sent has to adhere to a very strict layout - and the prepended non-PHP content breaks this structure, destroying the graph. In my very own case, all that had happened was that I had a trailing space and line feed at the end of one of my plugins. The extra two bytes shifted the magic number, and no reader could identify the file. It seems the server does not send trailing line feeds if those are the only trailing content; add anything else, however, such as a leftover space or tab character, and the graph will be destroyed. While in my case a plugin was the culprit, and issues in core files would become apparent quickly, the fact remains that this issue is not limited to plugins - any file loaded before GraphViz's image data is sent to the client can cause this. A single trailing space in a core file could take out graphs for the entire userbase, if it's pushed out. The solution should be not loading all the unnecessary stuff in the first place, however, as a quick measure, wrapping the plugin loader (plugin_register_installed()) in an output buffer and discarding it afterwards works. I have attached such a patch against 1.2.3.
Tags	evil, graphs, plugins
Attached Files	plugin_graph.patch (663 bytes) * plugin_api.php.virgin 2010-09-14 20:40:10.000000000 +0200 --- plugin_api.php 2010-09-29 07:36:29.000000000 +0200 *********** * 770,781 ** --- 770,783 ---- $t_query = "SELECT basename, priority, protected FROM $t_plugin_table WHERE enabled=" . db_param() . ' ORDER BY priority DESC'; $t_result = db_query_bound( $t_query, Array( 1 ) ); + ob_start(); while( $t_row = db_fetch_array( $t_result ) ) { $t_basename = $t_row['basename']; plugin_register( $t_basename ); $g_plugin_cache_priority[$t_basename] = $t_row['priority']; $g_plugin_cache_protected[$t_basename] = $t_row['protected']; } + ob_end_clean(); } / plugin_graph.patch (663 bytes)

atrol 2010-09-29 04:01 developer ~0026903	Thanks for this really good explaination What do you think about a patch which does not hide the trailing problem but exposes it? For example the way it's done in core.php if ( ($t_output = ob_get_contents()) != '') { echo 'Possible Whitespace/Error in Configuration File - Aborting. Output so far follows: '; echo var_dump($t_output); die; }

Renegade 2010-09-29 04:53 reporter ~0026905 Last edited: 2010-09-29 04:56	Personally, I wouldn't mind either way - the issue I'm seeing is with end users and less knowledgeable plugin writers. Basically, getting an error message like that helps if you know what to do about it. If you don't, you're still stuck with it despite getting more help than I had, and still have to contact others for help. On the other hand, there's really no reason to allow additional output to end up in the image data anyway - there's nothing else supposed to be there. It's a question of telling the coder not to do that vs. not allowing him to do that in the first place. Very most likely, dying and yelling will minimize the problem - but it'll also leave room for a confused minority who's getting yelled at by the server and doesn't know what to do, while simply discarding extra data would leave no room for confusion - the error simply can't happen. In addition, you have to consider legacy issues. The plugin in question had been running for several weeks without problem. I only noticed this issue when I tried to turn on graphs. There could be dozens of plugins on hundreds of installations out there with a problem like this, and if you just die() in the next version, their owners are going to be very confused. If you do go with a warning, though, you should make the error message very specific. At best, tell them in which file or in which plugin they should remove all content outside of <?php ?> and all output to STDOUT. The evil of this issue is that, unless you happen to have whitespace markers turned on in your editor, the culprits are invisible, and since some text editors automatically append a line break at the end of the file, it might not even be the coder's fault. As said: I, personally, don't mind either way, but I do think quietly swallowing the output will drastically reduce the probability of issues. EDIT: Probably clear from the description, but just for completeness' sake, I'd like to point out that this is of course not limited to trailing content - putting stuff before <?php will of course also trigger this.

atrol 2010-09-29 05:30 developer ~0026907	Reminder sent to: dhx, jreese Please comment this. Any solution of the problem would help to avoid big waste of time when searching for bugs.

jreese 2010-09-29 12:36 reporter ~0026916	There's a very simple, and widely recommended, solution to make sure this doesn't happen. Don't put the final ?> tag at the end of your PHP files unless you explicitly want to output some data afterwards. Leaving the end tag out ensures that your PHP file can never have extra output that goes unnoticed, and is utilized everywhere in the core of Mantis, and should be utilized everywhere in plugins as well. Adding extra output buffering to "catch" errant output is just a waste of processing time on every single page to fix a problem that's better solved by simply typing fewer characters in your code.

atrol 2010-09-29 13:14 developer ~0026917	I am a friend of anything which helps to have a good performance. Your arguments would also lead to the decision, that the existing code in core.php should also be removed. But there was probably a good reason to introduce this check. What about to check for trailings whitespaces (and maybe some more checks) just one time when installing the plugin? This would not prevent, when someone changes a plugin after installing, but most of the users won't do that.

Renegade 2010-09-29 14:00 reporter ~0026918 Last edited: 2010-09-29 14:04	While a quick look around Google confirms what jreese says about this practice, I do think closing this as won't fix is counterproductive. The probability that a random admin will make the mental step from "my graphs are broken" to "silly me, I should have omitted the PHP end tag!" is rather low, imo. Especially because, on first glance, the two have nothing to do with each other. If "stock image files" in Mantis are broken, why would I check the whitespace in my personal plugins, you know? If this issue isn't handled on the code side, it'd be a good idea to at least drop a note in an FAQ or in the plugin/config documentation, so people can search for the issue and find a solution if they encounter it.