News:

Do you need help?
Simutrans Wiki Manual can help you to play and extend Simutrans. In 9 languages.

Some Undefined Behaviour fixes

Started by ceeac, July 18, 2019, 08:32:42 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

This is the first in a series of patches aimed at fixing Undefined Behaviour (reported by gcc/clang -fsanitize=undefined).
This first patch fixes a signed integer overflow in int_noise, a null pointer dereference and an uninitialized read during game startup.

Dwachs

The debug-message in binary-heap  serves no purpose. I could be deleted.
Parsley, sage, rosemary, and maggikraut.

Ters

This being C++, I wonder if C++-style functional casts would be better than C-style casts. That is uint32(x) rather than (uint32)x.

An_dz

We should probably not be doing so many casts to begin with.

Quote from: Dwachs on July 18, 2019, 11:40:05 AM
I could be deleted.
Do you want to keep anything from your account before I delete it? :D

Dwachs

Parsley, sage, rosemary, and maggikraut.

Dwachs

Parsley, sage, rosemary, and maggikraut.

ceeac

Quote from: Ters on July 18, 2019, 03:40:04 PMThis being C++, I wonder if C++-style functional casts would be better than C-style casts. That is uint32(x) rather than (uint32)x.
Using static_cast would be the most idiomatic C++ solution, but the code base already uses a lot of c-style casts, so at least it is consistent now.

Ters

Quote from: ceeac on July 20, 2019, 02:27:22 PM
Using static_cast would be the most idiomatic C++ solution, but the code base already uses a lot of c-style casts, so at least it is consistent now.
That's ridiculously long. And if that was what C++ intended, why did they come up with the shorter one?

DrSuperGood

As far as I am aware C style casts are fully supported by C++. They convert into a well defined C++ style cast and will call the appropriate methods based on the desired result type. The C++ casts only need to be used if the conversion has some ambiguity (compiler might perform incorrect one) or if one wants to limit to an explicit type of cast to throw a compile time error if its mechanics are violated.

https://en.cppreference.com/w/cpp/language/explicit_cast

ceeac

Quote from: Ters on July 20, 2019, 02:29:23 PMThat's ridiculously long.
I believe it is in part intentional, to make casting more explicit, and to discourage too much casting altogether.

Quote from: Ters on July 20, 2019, 02:29:23 PMAnd if that was what C++ intended, why did they come up with the shorter one?
It's probably due to template stuff; if you have something like

template<typename T>
T foo()
{
    return T(3);
}

T can be e.g. of type int or a user-defined type.

Ters

Quote from: DrSuperGood on July 20, 2019, 04:17:27 PMAs far as I am aware C style casts are fully supported by C++.
What triggered my question was that C-style casts introduce one extra set of parentheses into what is already quite a mess of parentheses.

Quote from: ceeac on July 20, 2019, 04:47:19 PMThey convert into a well defined C++ style cast and will call the appropriate methods based on the desired result type. The C++ casts only need to be used if the conversion has some ambiguity (compiler might perform incorrect one) or if one wants to limit to an explicit type of cast to throw a compile time error if its mechanics are violated.
I think that last one is quite a good reason for not using C-style casts. They can turn into static_casts or reinterpret_casts, and while it is very well defined and not random which, it is quite easy for a developer to get a reinterpret_cast they don't want. Although I think this will only be a problem with pointers, and maybe references. It does appear that functional casts are just as unsafe, you just have to define a named type for a pointer-type to get reinterpret_cast. In this particular case however, I was just wondering about readability, not type-safety. Maybe that is functional casts sole purpose of existence.

Quote from: ceeac on July 20, 2019, 04:47:19 PM
I believe it is in part intentional, to make casting more explicit, and to discourage too much casting altogether.
It's probably due to template stuff; if you have something like

template<typename T>
T foo()
{
    return T(3);
}

T can be e.g. of type int or a user-defined type.
All other casts work just as well in that case. Although as mentioned above, it is prone to being used as foo<int *>().

prissi

The int_noise is strange. First, there are much more other places with possible overflows and loss of precision all over the code. Furthermore, in this occasion it does no matter of signed or unsigned, since only 31 bits are used. So what is the point of making it usnigned?

Ters

Quote from: prissi on July 21, 2019, 02:26:54 PM
The int_noise is strange. First, there are much more other places with possible overflows and loss of precision all over the code. Furthermore, in this occasion it does no matter of signed or unsigned, since only 31 bits are used. So what is the point of making it usnigned?
The point is to stop something from complaining that signed and unsigned integers are mixed. Looking more closely at it, I think the correct thing to do is cast noise_seed to signed integer, not everything to unsigned. It is hard to tell with such cryptic code. That means there is a signed left-shift again.