News:

SimuTranslator
Make Simutrans speak your language.

Fatal: Opening convoy Details panel causes crash.

Started by DrSuperGood, February 10, 2018, 06:40:33 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

DrSuperGood

As always this applies to the current nightly build at the time of posting.

Opening any convoy Details panel causes the Simutrans Extended client to crash with a vector index out of bounds error. Can be any convoy on the demo map or the server game, the client always crashes with the error.

Looking at the error and how it crashes the 32bit build of the client my theory is some kind of infinite allocation loop. The 64bit client eventually allocates an array over 4 GB long and crashes with an in game error message. The 32bit client silently freezes.

This is a high priority fatal error to fix. One needs access to the detail window to withdraw or alter the prices of individual convoys. It is extremely likely players will run into this.

ACarlotti

The issue lies with the penultimate commit: passing a numerical argument cap to a vector_tpl constructor allocates memory for cap objects, but does not initialise them and still returns an empty vector.

Reverting the following change in vehicle/simvehicle.cc should fix this. I'm not entirely sure why James changed this, as using an ordinary array in those circumstances seems consistent with what happens elsewhere in the codebase.

So, if you can compile yourself, then you can try this. Otherwise we'll have to wait for James.

@@ -2103,7 +2105,7 @@ void vehicle_t::get_cargo_info(cbuffer_t & buf) const
        const goods_desc_t* ware_desc = get_desc()->get_freight_type();
        const uint16 menge = get_desc()->get_total_capacity();
       
-       vector_tpl<ware_t> fracht_array[number_of_classes];
+       vector_tpl<vector_tpl<ware_t>> fracht_array(number_of_classes);
        for (uint8 i = 0; i < number_of_classes; i++)
        {
                FOR(slist_tpl<ware_t>, w, fracht[i])


jamespetts

#2
Thank you for the report and the diagnosis. I modified this because the original does not compile with Visual Studio 2015 (is this a C++17 language feature?), as number_of_classes is not a compile-time constant. I had not realised that this statement was intended to initialise, rather than simply declare, the array of vectors. I think that this needs to be done with new/delete, but this heap allocation is horribly inefficient.

Edit: Apparently, these dynamic arrays are not part of the C++ standard, even though they compile in GCC.

Edit 2: The only way that I can think to solve this without using new/delete is as follows:


vector_tpl<vector_tpl<ware_t>> fracht_array(number_of_classes);
    for (uint8 i = 0; i < number_of_classes; i++)
    {
        vector_tpl<ware_t> this_iteration_vector(fracht->get_count());
        FOR(slist_tpl<ware_t>, w, fracht[i])
        {
            this_iteration_vector.append(w);
        }
        fracht_array.append(this_iteration_vector);
    }


Can anyone think of a more efficient implementation? Note that this part of the code is not performance critical. I have tested this, and it does solve the crash.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

The easiest fix, surely, is just to make the array length 255 instead of number_of_classes.

jamespetts

Quote from: ACarlotti on February 10, 2018, 03:54:38 PM
The easiest fix, surely, is just to make the array length 255 instead of number_of_classes.

Yes, that could also work.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ACarlotti

I've just realised I misread "does solve the crash" as "does not solve the crash". So what you have done is what I probably also would do, except that I think fracht->get_count() is the number of classes, which doesn't make sense where you put it. Just let it allocate no capacity by default.

DrSuperGood

Apparently there were conflicting point of views when it comes to stack allocation. Dynamic sized stack elements are a C feature, but not a C++ feature. C++ considered the entire idea unsafe, error prone or inefficient, I am not sure which.

The problem likely stems due to how such dynamic sized elements are implemented. With constant sized elements one knows exactly where they are located, in the form of a constant offset, from the current stack pointer. With dynamic sized elements one has to push extra stack frames and make sure they are returned which can greatly complicate the generated code and lower performance in unintuitive ways. If one fails to return from stack frames correctly one gets a stack corruption. This also forces ordering to some extent, so large arrays that are no longer required might have to be retained on the stack until the function returns as another dynamic sized frame was added above them, where as if they were allocated on the heap they could be freed the instant they are no longer useful.

Since you are dealing with the stack allocating some large constant sized array should be fine as long as the function is not iterative and the size is still reasonable. 255*4 would be 1020 bytes which is 1 KB which should still be fine for modern stacks but can be considered quite large for a stack.

jamespetts

Excellent - very interesting information, thank you.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

DrSuperGood

Tests on todays nightly show this to be fixed. Convoy detail panel opens without crashing.

Also on an amusing side note, this topic must have triggered some kind of common search result as it has been read ~750 times as of posting this.

jamespetts

Splendid, thank you for confirming.

(I have no idea what the common search result might be - these things can be a little random at times).
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.