The International Simutrans Forum

Development => Patches & Projects => Incorporated Patches and Solved Bug Reports => Topic started by: ceeac on July 18, 2019, 08:32:42 AM

Title: Some Undefined Behaviour fixes
Post by: ceeac on July 18, 2019, 08:32:42 AM
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.
Title: Re: Some Undefined Behaviour fixes
Post by: Dwachs on July 18, 2019, 11:40:05 AM
The debug-message in binary-heap  serves no purpose. I could be deleted.
Title: Re: Some Undefined Behaviour fixes
Post by: Ters on July 18, 2019, 03:40:04 PM
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.
Title: Re: Some Undefined Behaviour fixes
Post by: An_dz on July 18, 2019, 08:59:28 PM
We should probably not be doing so many casts to begin with.

I could be deleted.
Do you want to keep anything from your account before I delete it? :D
Title: Re: Some Undefined Behaviour fixes
Post by: Dwachs on July 19, 2019, 07:35:19 AM
:) I -> it
Title: Re: Some Undefined Behaviour fixes
Post by: Dwachs on July 20, 2019, 10:16:30 AM
thanks, incorporated in r8791
Title: Re: Some Undefined Behaviour fixes
Post by: ceeac on July 20, 2019, 02:27:22 PM
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.
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.
Title: Re: Some Undefined Behaviour fixes
Post by: Ters on July 20, 2019, 02:29:23 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?
Title: Re: Some Undefined Behaviour fixes
Post by: DrSuperGood on July 20, 2019, 04:17:27 PM
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 (https://en.cppreference.com/w/cpp/language/explicit_cast)
Title: Re: Some Undefined Behaviour fixes
Post by: ceeac on July 20, 2019, 04:47:19 PM
That's ridiculously long.
I believe it is in part intentional, to make casting more explicit, and to discourage too much casting altogether.

And 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
Code: [Select]
template<typename T>
T foo()
{
    return T(3);
}
T can be e.g. of type int or a user-defined type.
Title: Re: Some Undefined Behaviour fixes
Post by: Ters on July 20, 2019, 06:46:14 PM
As 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.

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.
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.

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
Code: [Select]
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 *>().
Title: Re: Some Undefined Behaviour fixes
Post by: 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?
Title: Re: Some Undefined Behaviour fixes
Post by: Ters on July 21, 2019, 04:43:33 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.