The International Simutrans Forum

 

Author Topic: Some Undefined Behaviour fixes  (Read 855 times)

0 Members and 1 Guest are viewing this topic.

Offline ceeac

  • *
  • Posts: 48
Some Undefined Behaviour fixes
« 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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4603
  • Languages: EN, DE, AT
Re: Some Undefined Behaviour fixes
« Reply #1 on: July 18, 2019, 11:40:05 AM »
The debug-message in binary-heap  serves no purpose. I could be deleted.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: Some Undefined Behaviour fixes
« Reply #2 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.

Offline An_dz

  • Web Admin
  • Administrator
  • *
  • Posts: 2908
  • D'oh
    • by An_dz
  • Languages: pt, en, it, (de)
Re: Some Undefined Behaviour fixes
« Reply #3 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

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4603
  • Languages: EN, DE, AT
Re: Some Undefined Behaviour fixes
« Reply #4 on: July 19, 2019, 07:35:19 AM »
:) I -> it

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4603
  • Languages: EN, DE, AT
Re: Some Undefined Behaviour fixes
« Reply #5 on: July 20, 2019, 10:16:30 AM »
thanks, incorporated in r8791

Offline ceeac

  • *
  • Posts: 48
Re: Some Undefined Behaviour fixes
« Reply #6 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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: Some Undefined Behaviour fixes
« Reply #7 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?

Offline DrSuperGood

  • Dev Team
  • Devotee
  • *
  • Posts: 2727
  • Languages: EN
Re: Some Undefined Behaviour fixes
« Reply #8 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

Offline ceeac

  • *
  • Posts: 48
Re: Some Undefined Behaviour fixes
« Reply #9 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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: Some Undefined Behaviour fixes
« Reply #10 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 *>().

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9568
  • Languages: De,EN,JP
Re: Some Undefined Behaviour fixes
« Reply #11 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?

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5543
  • Languages: EN, NO
Re: Some Undefined Behaviour fixes
« Reply #12 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.