Most are very unlikely to ever be triggered in our codebase; two
stand out: linkgraph and money cheat. Those, potentially, could
wrap earlier than expected.
Basically, you could setup an auto-replace in a group for trains
to replace a ship with another ship.
Most of the code is surprisingly okay with this, it is only the
group statistics that doesn't like this.
You could give a wagon in the chain to reverse (which makes no
functional sense ofc). In result, only parts of the vehicle were
reversing, leading to weird crashes.
The bug comes in two slices:
1) the functions never actually checked if "tile" was a depot tile.
This allowed executing the function on tile 0, where are the
things like shadows of aircrafts are.
2) BuildDepotVehicleList() first checked if a vehicle is in a depot
before checking if it was a primary vehicle. This is invalid
for aircraft.
Fixing the first hides the second, and fixing the second makes the
first non-exploitable. But, fixing both felt like the best thing
to do.
Height was a unsigned 32bit integer, where TileAddWrap uses a
signed 32bit integer for the height. In result, there was an
implicit cast from unsigned to signed, messing things up.
But looking at it from a functional perspective, allowing such
large values is not sensible. In fact, width is restricted to
just a 8bit integer. By changing height to a 8bit integer too,
the implicit cast will never make a positive value negative anymore.
It first checked if the vehicle was in the depot, which for some types
is only a valid action for the primary vehicle. Afterwards, it checked
if the vehicle was a primary vehicle.
A race condition happens when an IPv6 connection takes more than
250ms to report an error, but does return before the IPv4 connection
is established.
In result, an invalid socket might be used for that connection.
It turns out that having "-g" in the compile-statement causes
Emscripten to pick -g3, which makes for very big binaries. This
is very likely not your intention when building Emscripten, as
smaller really is better.
For comparison, with RelWithDebInfo the binary is ~80MB. With
Release it is ~7.4MB.
This gives user visual feedback that the refresh is still pending, and
prevents people from clicking again and again thinking nothing is
happening. This is especially true for connections that fall back to
TURN, as that takes a few seconds to kick in.
Additionally, prevent clicking on the button again while a refresh
is pending. This is only delaying a successful result.
Although several places were fixed during the PR making the change,
not all made it in this document.
While at it, removed all kinds of Markdown warnings by an excessive
usage of spacebar in this document.
The function clears all stun-handlers. This causes all of those
objects to be destroyed.
A handler can have a pending connecter, which was only killed in
case CloseConnection() was called. This is never the case when
the object is destroyed. In result, the connecter could finish
and cause a use-after-free by calling into the (now deleted)
handler.
"stations_near" wasn't updated when founding a town near
a station. As this variable is not saved, any client joining
after the town is founded has a different value for
"stations_near", potentially causing desyncs.
As the intention of this if() statement was to skip an expensive
calculation when there are clearly no stations, better to move
that check inside the function, so other places also enjoy
the speedup.
When coming across any docking tile (for example, all tiles around
an oilrig are docking tiles), it always at least added a penalty
of 3 times a normal tile, even when there are no ships on them.
In result, the pathfinder got suggested to always go around docking
tiles. This was most likely not the intention of the change made in
31db4f8d5e.
When you are query several servers at once, it is rather unclear
for which server you got a popup. Instead, show any errors on the
server itself.
This is only true for the query-part. Joining a server still gives
an error popup to tell you about any issue.
You can now still query a full server, as long as the maximum
amount of allowed connections isn't reached. This means that as
long as there are not 255 clients connected to a server, you can
always connect to query.
Old servers don't tell the GameScript they are running, so nothing
should be shown.
All values in NetworkGameInfo initialize as 0/empty, except for GS
version. Someone has to be different from the rest, I guess.
One question that keeps popping up: "when do we release 2.0?".
NewGRF will force that at least 1.16 will be 2.0, but to not wait
for this, let's drop the "1." and be for ever done with that
conversation.
We are following in the footstep of giants here.
Nobody really paid attention to the lobby window, and it completely
missed its purpose. Most people don't even wait for companies to
show up, but just hit "New Company".
This in turn means people create a lot of unneeded companies, while
they "just want to watch the game" or join another company.
Instead, "Join Game" now just joins the game as spectators.
Soon we will make "join game" join the game as spectator first,
so limiting the amount of spectators makes no sense anymore in
that context. Not sure it ever did make sense.
"my_client" wasn't always free'd when a game ended. "my_client"
keeps a reference inside the PT_NCLIENT pool. The rest of the
code assumes that when you are not in a game, it can freely
reset this pool.
In result: several ways to trigger a use-after-free.
TURN is a last resort, used only if all other methods failed.
TURN is a relay approach to connect client and server together, where
openttd.org (by default) is the middleman.
It is very unlikely either the client or server cannot connect to
the STUN server, as they are both already connected to the Game
Coordinator. But in the odd case it does fail, estabilishing the
connection fails without any further possibility to recover.
This method doesn't require port-forwarding to be used, and works for
most common NAT routers in home setups. But, for sure it doesn't work
for all setups, and not everyone will be able to use this.
Every outgoing connection, either TCP or UDP, triggered
NetworkInitialize(), which triggered NetworkUDPInitialize() which
first closes all connections.
Now the problem was that "Search LAN games" found a server, added
it to the list, after which (over TCP) it queries the server. This
closes all UDP sockets (as that makes sense, I guess?), while the
UDP was still reading from it.
Solve this by simply stop initializing UDP every time we make an
outgoing TCP connection; instead only do it on start-up.
In this mode you do register to the Game Coordinator, but your
server will not show up in the public server listing. You can give
your friends the invite code of the server with which they can
join.
This removes the need to know a server IP to join it. Invite codes
are small (~7 characters) indentifiers for servers, which can be
exchanged with other players to join the servers.
Normally TCPConnecter will do a DNS resolving of the connection_string
and connect to it. But for SERVER_ADDRESS_INVITE_CODE this is different:
the Game Coordinator does the "resolving".
This means we need to allow TCPConnecter to not setup a connection
and allow it to be told when a connection has been setup by an external
(to TCPConnecter) part of the code. We do this by telling the (active)
socket for the connection.
This means the rest of the code doesn't need to know the TCPConnecter
is not doing a simple resolve+connect. The rest of the code only
cares the connection is established; not how it was established.
This statement was removed by accident, as it felt it could be removed.
But it is used to know if the NewGRF is from the baseset folder or
from the NewGRF folder.
This reduced the load on compilers, as currently for example MacOS
doesn't like the huge settings-tables.
Additionally, nobody can find settings, as the list is massive and
unordered. By splitting it, it becomes a little bit more sensible.
LoadCheck makes it sound like something is really broken while
loading savegames, while it really is perfectly normal, as most
chunks do not implement LoadCheck.
num_liveries indirectly contained the same information, but this
makes reading these things pretty difficult. So use IsSavegameVersionBefore()
like everywhere else instead.
IsSavegameVersionUntil() did a [0, N] check, not [0, N) as the
name suggests.
Until can be a confusing word, where people consider it to be
including the upperbound. Dictionary states it means "before",
excluding the upperbound. There are long debates about who is right.
So, simply remove away from this ambiguity, and call it "before"
and "before or at". This makes the world easier for everyone.
We no longer need them. If you want to remove a field .. just
remove it! Because of the headers in the savegame, on loading,
it will do the right thing and skip the field.
Do remember to bump the savegame version, as otherwise older
clients can still load the game, but will reset the field you
have removed .. that might be unintentially.
We won't be able to make it fully self-descriptive (looking at you
MAP-chunks), but anything else can. With this framework, we can
add headers for each chunk explaining how each chunk looks like
in detail.
They also will all be tables, making it a lot easier to read in
external tooling, and opening the way to consider a database
(like SQLite) to use as savegame format.
Lastly, with the headers in the savegame, you can freely add
fields without needing a savegame version bump; older versions
of OpenTTD will simply ignore the new field. This also means
we can remove all the SLE_CONDNULL, as they are irrelevant.
The next few commits will start using this framework.
We often ask people for their openttd.cfg, which now includes their
passwords, usernames, etc. It is easy for people to overlook this,
unwillingly sharing information they shouldn't.
By splitting this information over either private.cfg or secrets.cfg,
we make it more obvious they shouldn't be sharing those files, and
hint to what is inside them.
Instead of creating the object on heap and use a pointer, create
the object on stack and use a guaranteed-not-null pointer.
The size of IniFile doesn't warrent the forcing to heap.
Additionally, use a subclass instead of a function to do some
initial bookkeeping on an IniFile meant to read a configuration.
ThreadSanitizer rightfully notices that the game-thread could
update the palette while the draw-thread is copying it for local
use. The odds of this are very small, but nevertheless, it does
carry a very good point.
It wouldn't hurt the application in any way, but it might cause
visual glitches on the screen.
