Exception in destructor

[seemoritz]

NodeRefListBuilder and other classes in osmium/builder/osm_object_builder.hpp call add_padding() in their destructors.
add_padding can throw an osmium::buffer_is_full exception if the buffer is not allowed to auto_grow. (and at least a bad_alloc otherwise)

Destructors are implicitly declared noexcept since C++11 which leads to a program termination.
Declaring the destructors noexcept(false) is not a solution either, since a buffer_is_full exception can be the cause of the objects destruction, a second throw during exception propagation would therefore terminate the program anyway.

I think the add_padding needs to be moved outside of the destructor.

[joto]

add_padding() should never throw because it should never need more memory than there is available. The buffers capacity should always be a multiple of the alignment and the add_padding() never needs more than what is available to that alignment boundary. Do you have a specific scenario where you think this is not the case?

[seemoritz]

I ran into this problem, when I tried to set the buffer to a fixed size and would write and realloc it on a thrown buffer_is_full exception.
I used NodeBuilder and TagListBuilder to write nodes with tags into this buffer.
This worked for a small example but terminated on a bigger one (with lots of nodes).
I will try to find a minimal example that can reproduce this behaviour.

[seemoritz]

>_< didn't mean to close the issue there ...

Will reopen, when I have an example ...

[seemoritz]

I have now written an example function, that will crash the program if executed with this input:
monaco.txt

The OSM file monaco.txt is exactly long enough to fill the buffer.
If you remove one letter from the value of the last tag, it will not crash.

(Tested with v2.11.0/master-branch on Windows with Visual Studio Community 2015)

#include <osmium/io/any_input.hpp>
#include <osmium/io/any_output.hpp>

#include <iostream>
#include <string>

namespace osm{
struct Tag {
    std::string key, value;
    Tag(std::string key, std::string value)
        : key{std::move(key)}, value{std::move(value)} {}
};
}

void break_me() {
    osmium::io::File in_file("monaco.txt", "osm");
    osmium::io::Reader reader(in_file);

    // this is where i write to - would be writte by an osmium::io:Writer
    // but will crash before that
    osmium::memory::Buffer write_buffer{10 * 1024,
                                        osmium::memory::Buffer::auto_grow::no};

    while (osmium::memory::Buffer buffer = reader.read()) {
        for (auto& it : buffer) {
            if (it.type() == osmium::item_type::node) {
                auto& node = static_cast<osmium::Node&>(it);
                std::vector<osm::Tag> tags;
                for (auto& tag : node.tags()) {
                    tags.emplace_back(tag.key(), tag.value());
                }

                try {
                    osmium::builder::NodeBuilder builder{write_buffer};
                    builder.add_user("123abc123");
                    osmium::Node& obj = builder.object();
                    obj.set_id(node.id());
                    obj.set_location(osmium::Location{node.location().lon(),
                                                      node.location().lat()});
                    {
                        osmium::builder::TagListBuilder tl_builder{write_buffer,
                                                                   &builder};
                        for (const auto& tag : tags) {
                            tl_builder.add_tag(tag.key, tag.value);
                        }
                    }
                } catch (...) {
                    // This should catch the buff_is_full exception
                    // but instead the program will terminate
                    std::cerr << "there was an exception, which I caught!";
                }
            }
        }
    }
}

Edit: forgot to include osm::Tag struct

[joto]

Not sure what is going on there. I can not reproduce this on Linux. I can't get a terminate under any circumstance, programs runs just fine to completion. If I make the buffer smaller, the message in the catch clause will be printed.

Are you compiling this in Dev or Debug mode? This should enable some asserts that might help us find the problem.

[seemoritz]

In the code, do you make any assumptions about the sizes of structs, which may not hold under a different compiler?
I will try to compile in Debug mode, and see if I get any useful output.

[seemoritz]

I've finally managed to crash it under Linux.

Try the following input:

<?xml version='1.0' encoding='UTF-8'?>
<osm version="0.6" generator="">
  <node id="3735928559" lat="43.7311424" lon="7.4197576">
    <tag k="1234567890xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx088888888" v="1"/>
    <tag k="k" v="v1234"/>
  </node>
</osm>

with this program (changed the buffer size to 128):

#include <osmium/io/xml_input.hpp>

#include <iostream>
#include <string>

namespace osm{
struct Tag {
    std::string key, value;
    Tag(std::string key, std::string value)
        : key{std::move(key)}, value{std::move(value)} {}
};
}

void break_me() {
    osmium::io::File in_file("monaco.txt", "osm");
    osmium::io::Reader reader(in_file);

    // this is where i write to - would be writte by an osmium::io:Writer
    // but will crash before that
    osmium::memory::Buffer write_buffer{128,
                                        osmium::memory::Buffer::auto_grow::no};

    while (osmium::memory::Buffer buffer = reader.read()) {
        for (auto& it : buffer) {
            if (it.type() == osmium::item_type::node) {
                auto& node = static_cast<osmium::Node&>(it);
                std::vector<osm::Tag> tags;
                for (auto& tag : node.tags()) {
                    tags.emplace_back(tag.key(), tag.value());
                }

                try {
                    osmium::builder::NodeBuilder builder{write_buffer};
                    builder.add_user("123abc123");
                    osmium::Node& obj = builder.object();
                    obj.set_id(node.id());
                    obj.set_location(osmium::Location{node.location().lon(),
                                                      node.location().lat()});
                    {
                        osmium::builder::TagListBuilder tl_builder{write_buffer,
                                                                   &builder};
                        for (const auto& tag : tags) {
                            std::cout << "sizeof buffer " << write_buffer.written() << std::endl;
                            tl_builder.add_tag(tag.key, tag.value);
                        }
                        std::cout << "sizeof buffer " << write_buffer.written() << std::endl;
                    }
                } catch (...) {
                    // This should catch the buff_is_full exception
                    // but instead the program will terminate
                    std::cerr << "there was an exception, which I caught!\n";
                    std::exit(EXIT_FAILURE);
                }
            }
        }
    }
}

int main() {break_me();}

The problem happens, when therer are less than 8 Bytes left in the buffer, the Key of the tag fits into the remaining space, but the value does not.
An exception will be thrown and ~TagListBuilder() will try to add say 7 Bytes of padding.
This would work if the key had not already been written to the buffer.
The buffer thinks the key was already written, tries to add the bytes for the padding and throws another exception, which will terminate the program.

The first example did not crash on Linux, because nodes only take up 64 Bytes on Linux while they use 72 on Windows.

[springmeyer]

I've finally managed to crash it under Linux.

Can you provide a backtrace?

[joto]

Thanks @seemoritz for the detailed report. I could reproduce the problem and found the culprit. I'll provide a fix shortly.

[joto]

@seemoritz I have pushed a fix. Can you verify that it works for you now?

[seemoritz]

@joto I have applied your fix and run my test program as well as the normal one (with much higher throughput).
Both seem to work fine.