GNOME Bugzilla – Bug 753570
“double free or corruption” if a std::exception is thrown
Last modified: 2015-10-24 14:55:43 UTC
I found out, that libxml++ produces a "double free or corruption"-error, when exceptions of the basetype std::exception are thrown in the sax-parser-callbacks. (e.g. on_start_document, on_end_document, on_...) But it behaves normally, this means, that the exception can be catched, if an exception of basetype xmlpp::exception is thrown. Because it is very common, that exceptions with base-type std::exception are thrown (e.g. std::cout), it would be a good idea, to get all exceptions based on std::exception working, otherwise you should always add a try-catch block inside the sax-parser-callbacks. In my opinion this is not what I want from a library, because this would mean, that the code is not save, once you forget to encapsulate the whole callback with try-catch. If you don't think, you should support std::exception, maybe it would be a good idea, to document, that only xmlpp::exception exceptions are correctly handled in the sax-parser callbacks. #################################################################### MWE: #################################################################### #include <libxml++/libxml++.h> #include <iostream> class xml_parser : public xmlpp::SaxParser { protected: virtual void on_start_document() override; }; void xml_parser::on_start_document() { throw std::runtime_error("test-exception"); //throw xmlpp::internal_error("test-exception"); } int main(void) { xml_parser parser; try { parser.parse_memory( u8"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n"); } catch (std::exception &e) { std::cerr << "Caught exception: " << e.what() << std::endl; } } #################################################################### Compiled with: g++ main.cpp -o main `pkg-config --cflags --libs libxml++-2.6` -Wall -pedantic -Wextra -Werror -Wcast-qual -Wcast-align -Wconversion -fdiagnostics-color=auto -g -O2 -std=c++11 Expected Output: Caught exception: test-exception Output: *** Error in `./main': double free or corruption (!prev): 0x0000000000b35b30 *** Compiler: g++ 4.9.2 See also: http://stackoverflow.com/questions/31969961/libxml2-36-double-free-or-corruption-if-a-stdexception-is-thrown/31972852?noredirect=1#comment51854141_31972852
Created attachment 309185 [details] [review] Patch
Created attachment 309204 [details] [review] Patch with regression tests
I can see that there is a problem here, but I'm not fully happy with the proposed solution. Even with the attached patches, exceptions are sent through C code. Isn't that risky? I suspect that it may or may not work, depending on which compiler, or even version of compiler, is used. Glibmm tries to avoid such behaviour completely. I would feel more at ease if all exceptions were treated as similar to xmlpp::exception as possible. Perhaps replace catch(const exception& e) { parser->handleException(e); } by something like catch(const xmlpp::exception& e) { parser->handleException(e); } catch(const std::exception& e) { xmlpp::exception xmle(e.what()); parser->handleException(xmle); } catch(...) { xmlpp::exception xmle("Unknown exception"); parser->handleException(xmle); } Unfortunately there are lots of calls to parser->handleException(e). There will be lots of new code. Perhaps it's possible to find a smarter solution.
Sorry, but I don't understand the whole code, but I try to. So the reason that baseclass std::exception was created, was, to enable the possibility, to save the exception, before the code is executing plain C-Code, and than rethrow the saved exception, at check_for_exception(). If this was the purpose, why don't we use a cunstruct like the following (from http://en.cppreference.com/w/cpp/error/current_exception) try{ blah blah } catch(...) { parser->handleException(std::current_exception()); } and change the exeption functions in Parser.cc void Parser::handleException(std::exception_ptr eptr) { delete exception_; exception_ = eptr; if(context_) xmlStopParser(context_); } void Parser::check_for_exception() { check_for_validity_messages(); if(exception_) { std::exception_ptr tmp; //maybe a smart pointer here... _exception = nullptr; std::rethrow_exception(tmp); } }
Kjell: That's a good point about not throwing exceptions through C code. You are right that it is not safe. byteunit's idea to use std::exception_ptr and std::rethrow_exception() is ideal. However, the `exception* exception_' member in xmlpp::Parser should be changed to a std::exception_ptr member. Would this break ABI compatibility?
I agree that byteunit's idea is excellent. Obviously I'm not yet familiar with all the new possibilities in C++11. Avoiding an ABI break is the problematic part of it. Changing xmlpp::exception* exception_ to std::exception_ptr exception_ would definitely break ABI. Perhaps we can keep xmlpp::exception* exception_ in the definition of class xmlpp::Parser, but actually store a std::exception_ptr*, using reinterpret_cast where necessary. Disgusting, but it could be made to work. Except if some application program uses the protected exception_ data member in its own derived class. Possibly, we can change xmlpp::exception* to any other data pointer, in this case std::exception_ptr*, without breaking ABI. I'm not sure. At leat all data pointers have the same size.
But as far as I grepped, the Clone() and Raise() methods in the exception-classes are then not needed anymore, (the only calls to them would be eliminated by the fix) therefore It would be a good idea, if you can get rid of them in a future release.
I thought of another idea. Suppose that a wrapped_exception class is created which extends xmlpp::exception and contains the std::exception_ptr member. This should allow byteunit's idea to be implemented without affecting ABI compatibility. I'll see if I can get this to work.
Created attachment 309234 [details] [review] New patch that introduces xmlpp::wrapped_exception Per Comment #8
Thanks, Daniel, I think, that looks good.
Another excellent idea! This is the way to go before we can break ABI. Things can be simplified at the next API/ABI break. I will not have time to check and test the patch in detail right now. I'll be back before the end of August. Don't know if someone else will take care of it in the meantime.
Created attachment 309378 [details] [review] New patch with better testing
Thanks for the patch! I have pushed it with only a minor change: I removed // -*- C++ -*- from the new files. https://git.gnome.org/browse/libxml++/commit/?id=128615309454dc75ffcb9eaf83fe1188e35b5dfe However, both 'make check' after configuration with --enable-warnings=fatal and 'make distcheck' failed. I fixed that in a separate commit. https://git.gnome.org/browse/libxml++/commit/?id=e728c92fe1341caeda4838c7f416364da99c304a I also added comments to the new xmlpp::wrapped_exception class. https://git.gnome.org/browse/libxml++/commit/?id=23a566f084d9c119524a401dd748f741e9380586
Thanks Kjell. Your changes look good.
Unfortunately, std::exception_ptr is not available with g++ on all architectures: See bug #757042 .