On Moodle 2.0 - disabling stack traces to webbrowser?

On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
Number of replies: 25

Hi!

Hacking on HEAD (after very long!), I notice we now spit out big stack traces to the webbrowser (along with a good user-readable error).

This is not the best, we should be defaulting to sane and safe: stack traces and other diagnostic stuff must go to the error log. This way, we

  • Avoid revealing secrets to would-be attackers!
  • Can still report warnings and errors without breaking our output ("what's that PHP warning doing in the middle of XHTML strict?")
  • Can still report warnings and errors while writing output for non-tolerant (or non-display!) clients: for example WebDAV, AJAX and Web Services!
  • Can see the error messages. Even in HTML, our errors may end hiding in a corner, or disguised by the colour scheme.

In other words, our current default belongs to the times of PHP v3 smile

How can I switch this off? How can we change the defaults?

Edit: Note that Moodle is ignoring my settings:

 # log_errors is set to true
 # display_errors is set to false, and...
 $CFG->debug=38911;
 $CFG->debugdisplay=0;
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
Also -- when running code in setup.php and setuplib.php we _do not_ have access to fancy libraries and convenience...

...like "print_error()", which requires themes, localized stuff...
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
This is my code, and I thought the stack traces were appearing via debugging, which respected $CFG->debugdisplay.

Clearly I am making a wrong assumption there, can you work out where? I don't really want to look at this now.
Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
I managed to sort the problem out. I was using a config.php copied from a 1.9 install -- so the DB connection settings where "oldstyle".

I think that the DB connection code overrides the settings. It seems contained to that code, but it was truly weird. It was failing inside the theme engine (CFG->theme was not set) and if I fixed the theme problem, then blocklib was blowing up.

And it didn't say a thing about DB conn problems mixed

I do have some patches to the handling of debugging (not this particular problem, but one related to the WebDAV code) that I'd like to propose. Once I have the WebDAV stuff working, that is smile
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
I think it is because, suppose on course view.php, it calls print_error before output has started, we want that error in the course theme, so any error output needs to initialise the themes. Not ideal, but there it is.
Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
Ahhh, I was planning to change print_error() so that it throws exception instead. The reason is I need this for Web services, I need to register my own default error handler and instead of html output I need to send back XML with error description to client. I guess webdav should do the same...

In think there might be a better way to solve the debugging issues than what we did in 1.9, please cc me if you submit any patches into tracker, this is one of my top priorities now because I have to solve this in WS code too.
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
No, I don't want to code up and register "a logging handler".

PHP has support for this. We just have to use it. Right now the codepath for debugging stuff has gotten really long, and fragile.

When we are _debugging_, something has gone _wrong_. We cannot rely on a ton more PHP-script code, which is likely to fail as well.
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I was not talking about debugging, we have exactly the same problem with print_error() and exceptions the solution here. We are throwing exceptions in more and more places.

Hiding all debugging and sending it only to log is imho not a solution, I like the debug messages on my pages, sorry - I do not like when it breaks xhtml, but I do like it there. Ideally it should be redirected to page footer in debug mode+ the log of course.
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Frank Ralf -
What about using the YUI Logger?

Or FirePHP for sending debugging messages to the Firebug console (see also FirePHP plugin)?
Average of ratings: -
In reply to Frank Ralf

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I does not matter much, which technology we use - we just need to push the info into session or memory - our new messaging framework would be the right place to do this (I do not think it supports this kind of notification yet). I personally think the YUI logger is the way to go.
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -

Exactly, we have the same problem with print_error().

The very core code needs to send errors to the logs, and its error handling cannot have lots of dependencies or a deep stack trace.

What do I mean with this?

  • lib/setup*.php
  • all the database API
  • l10n/i18n code
  • installation code
  • the 'output' libraries
  • weblib

Why? Because if we have a problem in any of those, there isn't much you can count on. And by failing in the reporting path, it makes a mess that hides the real thing we are trying to report.

For modules and "higher level" code, it's ok. We can try harder to be helpful.

Hiding all debugging and sending it only to log is imho not a solution, I like the debug messages on my pages

It cannot be the default, for security reasons. And it cannot be the default because it breaks modern Moodle mixed

Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Martin, you like errors in your log. Some of us don't. The world of Moodle should be big enough for the both of us.

If an error occurs during the pretty print_error, then it should fall back to the print_early_error function, or whatever it is called these days, in setuplib.php.
Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -

It is big enough for us all smile but if you like errors in your output, there is a growing % of moodle functionality you can't hack on in that "mix errors and output" mode.

We should fix at least

  • $CFG->displaydebug=0 -- so I can use it fulltime and you can use it when you need it -- and trust me, you'll need it.
  • The handling of errors in core libraries.

I still think that it would be better from a security standpoint to set the default to put stacktraces only in the logs. But that's something that can be discussed later.

Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Dan Poltawski -
Not sure if this is really related to this discussion, but this has just reminded me to file MDL-20577 in relation to print_error displaying no output when in non-debug mode now. I've been meaning to file that bug for ages..
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
I've spent yesterday and a good part of today working on a diagnostic and cleanup.

When I said that having pretty exceptions was calling a ton of code, and making huge assumptions I has completely misunderestimating the situation.

The (inverted) callstack for the pretty exceptions is a bit like this

• you call print_error() -
• which calls $OUTPUT->fatal_error
• now, this wants to build a page, so you get moodle_core_renderer->fatal_error() which calls its own header() and render_page_template()
• which calls has_navbar() and navbar->has_items(), which in turn calls global_navigation->initialise() and from there global_navigation->load_for_category(), load_categories() and so on and so forth

From the warnings I see in my logs, it is also erroring out in blocklib -- so it must be trying to load up all the blocks. In the whole execution, there are gadzillion calls to has_capability() which lead to warnings and errors.

In summary -- the "pretty" path executes a ton of code, that makes insane assumptions. We are dealing with an error -- things are broken, gone, not working.

Take the shortest path and log this (print it if you will smile ) with the absolute least chance of other error happening. Because these cascading errors make the output completely uninteligible.

To wit -- I found what my error was: I had added a trailing "/" in my wwwroot. But Moodle was unable to report it.

I now have a full set of fixes, and will post a separate message about them. Basically, I am disabling the pretty errors during early init.

... however...

I think pretty errors right now are completely hosed. The assumption is that we hit a serious unexpected errror, but we can still execute. "If you have a serious car accident, drive your car to the nearest hospital."

Maybe we could have a pretty and minimal template page. No nav, no blocks, no nothing.

Barring that, I think it should be removed, as it obscures the real error unless the error is incredibly trivial.
Average of ratings: Useful (2)
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -

I have a brief patch series here http://dev.laptop.org/git/users/martin/moodle.git/log/?h=mdl20-errorhandling

With these fixes,

  • Early errors are handled correctly...in my testing at least. There will be cases that fool my 'is it early' detection... and early is not the right metric... should be "is it bad enough?".
  • debugdisplay is respected! smile
  • log_error (from php.ini) is respected!
  • All the error handling code logs first (if log_error is set), and then tries to do pretty-output, so at least the logs have a chance to show the real problem.
  • I think the 'recursing' check in outputlib can go or should be reworked. Need Tim's input on this.

Now, from the department of irony: Why do we show scary warnings about PHP's 'display_errors' setting if we default to... displaying errors? smile

Average of ratings: Useful (1)
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
hmmm, first of all could you please post links to normal patches instead of git log?

The current print_error() is not working correctly because it has to throw exception instead of printing anything and stopping. All the printing has to be done in default exception handler instead. I remember discussing this with Tim, but unfortunately there was not enough time. The problem is when you use print_error in any script that does not use HTML markup. With this exception approach you can define your own exception handler and send back any error data you want. I already used this approach in new WS framework where I need to send XML formatted errors.

Maybe we could just switch the exception handler on thy fly - one with printing simple errors, other initialised later when PAGE stuff present.

Petr
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
Yes, there is a lot of overlap between print_error() and the default exception handler. print_error() can probably just throw the exception as you say.

In any case, my patches made print_error() mimic the default exception handler.

Patches -- do read the commit message too!

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=d31554e1179e1f5ccb115e685d4823153c074237

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=f9f3574289f11711430cb75fd2dcb413e3904dea

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=f32c007daf55d752393325a0d074da6a71c44002

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=9997328db1823c5c1d61fd62d4b0d7fb5688f035

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=5cab67f6b5bc21e05193d0a09572e50e913b934f

http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=c0fbe5c2681675694e36b5ed471d678e00e4ddfb

Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I am not able to apply these to my HEAD CVS checkout. I need just one patch that applies to my CVS checkout. None of the IDEs I use supports git yet, sorry. If you do not want to exclude stubborn devs like me, please use patch formats compatible with the main VCS of the Moodle project smile
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Eclipse normally applies git patches just fine (you need to say Ignore 1 leading path segment so that it strips off the a/, b/ bit).

If you want one patch, you could get it in two or three commands

apt-get git ------------ or equivalent, if necessary.
git clone git://dev.laptop.org/users/martin/moodle.git
git diff 1e18b6fe65449..mdl20-errorhandling > patch.txt

There does not seem to be a way to do that through the web interface.

I promise this will not hurt, although there is a danger you might learn something in the process.

Heed Martin's advice and read the commit comments from each individual commit, if you have not already.

Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I know how to apply patches in Eclipse very well. Tried applying the patches ML linked one by one but it failed to merge it automatically because it could not find the matching parts, sorry. (This is not the first time I tried this and failed.)

I have tried your commands and it just gives "git diff [--no-index] takes two paths". Bad luck.

The fact there is no web interface for this means you have to include the patch for people who do not use git, sorry git is not official VCS yet - you have to expect not everybody is using it. You say it is simple for you to get that patch, fine, so why not attach it here together with the git links?

I decided to not learn the details of how gits works, after all somebody has to make sure the tutorials for Moodle Git migration actually work. I already read all the git related info in our docs wiki, so far I do not understand how it works and I am not able to use it wink

The same goes for IDEs, I am willing to test all git support in IDEs, so far I failed pretty badly. Egit in Eclipse seems to work better than Netbeans but I still think we are years away from fully working integrated git clients in IDEs. To me it looks like Linus and friends care only about command line tools and ignore the IDE folks, unsurprisingly the other side is doing the same. I think git is the perfect tool for Linux kernel development, but it still has a long way to go to be acceptable as a full replacement for CVS, SVN and similar for everybody.

So far it looks like the last 3 developers from our top ten list of contributors to persuade are me, Eloy and MartinD wink

Petr
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
There is usually a web UI that shows an arbitrary diff, in the "gitweb" package. OLPC is a different viewer ("cgit") for various reasons.

The version of cgit we use doesn't have the "show me arbitrary diffs" trick unfortunately.
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
My advice is don't use egit (Eclipse git plugin) yet - or at least use it with extreme care.

The update site page warns you not to; I ignored this and tried it for some personal work.

The plugin looks pretty good in general (history view, compare views, committing, changing branch, etc. all seemed to work), apart from needing some obvious polish. But more importantly, you do need to bear in mind that it isn't just the UI which is a pre-alpha version; the plugin relies on a pure-Java git implementation which is in similar status.

[This is probably why progress is relatively slow on the plugins. As I understand it, Git is written in a way that makes it hard to port to other platforms; in a cross-platform IDE you definitely want a pure Java version.]

Long story short, I told it to do a hard reset. That's git-speak for 'put things back to the last committed state', like 'replace with... latest from HEAD' when using CVS except that it always works on the whole tree. (Incidentally, those 'replace with' things are missing when using egit, which is one of the polish things I mentioned.) Anyway, it immediately bombed my repository back to the stone age several days previous. I think it discarded the entire branch I was working on. There was no sign of it or any of the checkins using normal git tools. This should not have happened. smile

Maybe it would've been possible to retrieve the lost commits from the repository somehow, but I was working on my Mac using Time Machine which was able to restore most of the data - I had altered very few files since the last hourly backup. Those altered files, I got back with Eclipse local history. I think I had to actually retype, like, ten lines of code. So, no big deal, but personally, I think I'll keep using the command-line git tools which have never lost me any data! Until the plugin reaches some version > 0.5 where they at least call it a beta, if not later.

Source management software is not where you want to be using buggy code. smile

--sam
Average of ratings: -
In reply to Petr Skoda

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Martín Langhoff -
Hi Petr,

I just tested the patches, they seem to apply perfectly well on today's CVS HEAD. Please check that you're passing "-p1" to the 'patch' command or its moral equivalent in Eclipse.

Ordering is also important. This is a patch series where some patches depend on the previous one. Each patch fixes one thing, and was individually tested. The result for the whole patchseries was also tested.

(In my post here, I ordered them "first to last". If you look at the git web view, the ordering is reversed, the topmost is the last one.)

The patch format is unified diff and is perfectly compatible with CVS.

You don't have to learn git to apply my patches.

Just follow the links I provided to the web-based patch browser, those show a very nice colour-coded unified diff. For a non-colour-coded, applicable patch you click once more, on the link named "patch". Should be easy enough.

If your webbrowser is mangling newlines, dos2unix can help. If you are using Windows and your CVS is mangling newlines, you may need unix2dos.

I am happy to help you in sorting out problems you may have applying those patches. But I'll need you to want to be helped for that to work smile
Average of ratings: -
In reply to Martín Langhoff

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Tim Hunt -
Picture of Core developers Picture of Documentation writers Picture of Particularly helpful Moodlers Picture of Peer reviewers Picture of Plugin developers
Now actually looking at the changes.

I don't like http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=d31554e1179e1f5ccb115e685d4823153c074237, because often 404 is more appropriate than 503 - for example when it was print_error('unknowncouresid', ...);

Sure sometimes 503 is more appropriate, but not always. Can we think of a better solution to this?


http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=f9f3574289f11711430cb75fd2dcb413e3904dea - I don't see the point of the 
if (!empty($CFG->debugdisplay)) {

test here. I think that test should be inside $OUTPUT->fatal_error.


Ditto in http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=f32c007daf55d752393325a0d074da6a71c44002, and a couple of the other commits.


http://dev.laptop.org/git/users/martin/moodle.git/patch/?id=f32c007daf55d752393325a0d074da6a71c44002 Writing to error log before doing the pretty message is good.


So, basically, the changes seem OK in intent to me, but contains too much duplication (i.e. too many !empty($CFG->debugdisplay) tests in different places.)

Any chance you could clean up this patch-series before we put it in HEAD.

Average of ratings: -
In reply to Tim Hunt

Re: On Moodle 2.0 - disabling stack traces to webbrowser?

by Petr Skoda -
Picture of Core developers Picture of Documentation writers Picture of Peer reviewers Picture of Plugin developers
I agree with most of the thinks TIm just said, I found some more duplication in problems in my original exception and error handling code.

Going to open new tracker issues and concentrate on the cleanup because I need to get this working asap for the web services layer.

Tracker issue: MDL-20676

Thanks for your valuable ideas and feedback.
Average of ratings: -