onsdag 19 augusti 2009

Error handling - Internal_error_handler to the rescue

In the beginning, error handling in MySQL code was a straightforward process. There was your C function my_error(int nr, int MyFlags, char* format, ...)that would simply put the error on a queue for you, ready to be returned to the client at the right time. The two last arguments work just like printf. You can easily check whether the current thread threw an error by reading THD::is_error(). It is very simple and very hard to get wrong.

Later down the line, however, the need has surfaced for something more. If module A asks module B to do something as part of the execution of module A, module B might set a critical error since this would be the right thing to do had it not been part of A's execution. Then later A might call on module C to do something, but there might be code inside C that is not being executed because it notices that somewhere something went wrong and bails out.

An example (from Bug#35996):
When the user issues SHOW CREATE VIEW, it might be that the view references non-existing tables. Why this is allowed is another discussion, beyond the scope of this post. There must also be an access check. The user has to have SELECT and SHOW CREATE privilege on the view, and SELECT privilege on all table columns that the view references.

The execution of SHOW CREATE VIEW will call on the check_access procedure, which recurses through the view itself and all tables or views that it references. This procedure is wired to raise an error for non-existing tables, and for columns that the user has no privileges an error will be raised saying

  • Table 'test.t1' doesn't exist, or
  • "... command denied to user ... for column ... in table ..."
respectively.

The latter error must be hidden by the SHOW CREATE VIEW execution since it gives out information about tables that the user has no right to know. Remember, the view may just do a SELECT *, but this error will actually name the columns for which the user is not authorized. The error must be replaced by the more anonymous

"View ... references invalid table(s) or column(s) or function(s) or definer/invoker of view lack rights to use them".

This is accomplished by examining the current error in the queue, then removing that error and replace it by the other one. . Recipe for disaster? Read on. Remember that SHOW CREATE VIEW allows the user to see the definition (and for the view to exist) of a view referencing columns or even tables that don't exist. Hence it must manipulate the errors once again. The same message may be used, but the status must be warning level and not error level. A message with an error status has the authority to stop the transfer of information to the client, while a warning will simply pop up after the wanted result is transferred. The problem is, by simply looking at the anonymous error message we don't know what the original cause is:
  • If it was a missing column, all is fine, let's downgrade an error to a warning an get on with it.
  • If it was a column that the user has no right to know about, there must be an error.
What to do?

Obviously, it is bad design to have functions putting up errors only so that other functions' execution paths depend on what errors are on the queue. You need to know why that error is there in the first place and what the circumstances were when it was put there. In other words, you need to be there when it happens.

Enter Internal_error_handler. This brilliant invention is a hook for whenever an error is raised and a caller may install such a hook before calling a function. This way an appropriate action may be taken so that the final error message may be set on the spot. It does this using a virtual method handle_error. (handle_condition in 6.0) Bug#35996 was fixed by making SHOW CREATE VIEW, prior to calling check_access, install an Internal_error_handler that would:
  • If the view itself was access checked when the error was raised, do nothing
  • If a column or a table was access checked when the error was raised, hide the details of it.
  • If the error was raised due to a missing column or table, hide the details of it and make it a warning.
Now is the time to take this approach one step further. The code that anonymized error messages is (for some reason) put in TABLE_LIST:
void TABLE_LIST::hide_view_error(THD *thd)

Post-bug#35996-fix the function is only called once, and in an awkward way. The fix to replace the code is trivial with lessons now learned. But a great amount of fragile code can get removed, spanning tens of files, so I will list the details below. Whenever a command is executed out on a view (e.g. SELECT, INSERT etc) referencing either non-allowed ot invalid columns/tables/databases, the error message gets anonymized. This is accomplished by a C-style function pointer in Name_resolution_context called process_error. This function constitues a hook for TABLE_LIST::hide_view_error. The hook is called from various places in the name resolution code, probably only the ones that happen to be passed in most current scenarios. Most likely you can find a number of bugs here. The hook is installed in mysql_make_view, however.

All of this code can be removed and replaced with the more general Internal_error_handler. The benefits are obvious: We replace an unsafe C pointer with polymorphism, we substitue a general approach for an ad-hoc one, we remove unrelated functionality from TABLE_LIST and we don't have to sprinkle the code with easily-forgettable calls to the process_error hook.

1 kommentar: