IRC logs for #openttd on OFTC at 2022-07-08
⏴ go to previous day
00:39:51 *** Montana has quit IRC (Quit: Leaving)
02:17:22 *** debdog has quit IRC (Ping timeout: 480 seconds)
02:18:09 *** D-HUND is now known as debdog
06:01:13 *** andythenorth has joined #openttd
06:24:49 *** sla_ro|master has joined #openttd
06:35:46 *** andythenorth has quit IRC (Quit: andythenorth)
06:40:57 *** andythenorth has joined #openttd
07:16:43 *** Tirili has quit IRC (Quit: Leaving)
07:18:08 *** urdh has quit IRC (Quit: Boom!)
07:21:33 *** andythenorth has quit IRC (Quit: andythenorth)
07:40:59 *** andythenorth has joined #openttd
07:56:46 *** andythenorth has quit IRC (Read error: Connection reset by peer)
07:57:17 *** andythenorth has joined #openttd
07:57:21 *** andythenorth has quit IRC ()
08:03:58 *** andythenorth has joined #openttd
08:28:56 *** Etua has quit IRC (Quit: Etua)
09:52:44 *** Etua has quit IRC (Quit: Etua)
11:07:46 *** WormnestAndroid has quit IRC (Read error: Connection reset by peer)
11:07:49 *** WormnestAndroid has joined #openttd
11:18:33 *** gelignite has joined #openttd
11:44:35 *** Smedles has joined #openttd
12:18:28 *** OsteHovel has joined #openttd
12:54:47 *** peignenappe has joined #openttd
12:55:47 *** peignenappe has quit IRC ()
13:25:47 *** nielsm has quit IRC (Ping timeout: 480 seconds)
13:49:09 *** Flygon has quit IRC (Read error: Connection reset by peer)
14:26:27 *** andythenorth has quit IRC (Quit: andythenorth)
15:01:07 *** Wormnest has joined #openttd
15:40:54 *** andythenorth has joined #openttd
16:03:38 *** HerzogDeXtEr has joined #openttd
16:33:34 *** Smedles has joined #openttd
16:54:06 *** HerzogDeXtEr has quit IRC (Read error: Connection reset by peer)
16:55:09 *** Flygon has quit IRC (Quit: A toaster's basically a soldering iron designed to toast bread)
17:34:28 *** andythenorth has quit IRC (Quit: andythenorth)
17:39:13 <Samu> it's official, now I know I need directions "per segment" based
17:46:55 <Samu> bad screenshot, some hidden text
17:47:52 *** WormnestAndroid has quit IRC (Ping timeout: 480 seconds)
17:48:54 <Samu> then on the bottom, before the last yellow line, it detects that tile is already on the list
17:49:17 <Samu> and then fails to add the segment in question
17:50:36 <Samu> SE_N_NW and SE_N_NE are 2 different neighbours that happen to have the same tile 945 with dir 64, but... for this matter, they were not supposed to forbid one another
17:52:30 <Samu> i need to also check segment_dir against segment_dir
17:53:01 <Samu> but i have a major issue with that, there's 20 different SegmentDirs
17:53:48 <Samu> tile directions are already consuming 16 bits
17:55:11 <Samu> and i need to place segmentdirs combined with tile dirs somehow
18:17:23 *** Wormnest has quit IRC (Ping timeout: 480 seconds)
18:23:19 *** WormnestAndroid has joined #openttd
18:24:46 *** WormnestAndroid has quit IRC (Read error: Connection reset by peer)
18:24:51 *** WormnestAndroid has joined #openttd
18:40:44 *** Wormnest has joined #openttd
18:55:56 <TrueBrain> "set p2 with the ClientID of the sending client." I love how we will have p1/p2 references till the end of days :P
19:17:02 <glx> well that's squirrel 3.0 doc, but some stuff still holds for 2.0
19:18:44 <glx> there's a "clone" expression, but the syntax in the doc is not very clear
19:20:19 <glx> documentation can mix parser syntax and examples in the same article
19:22:53 *** andythenorth has joined #openttd
19:25:41 <Samu> exp:= 'clone' exp, why is this example so difficult to understand
19:26:46 <glx> something like "local copy = clone original;" should work
19:31:40 *** ChanServ sets mode: +v tokai
19:32:16 <Samu> the advantage of table over AIList is that I can store more than just an int
19:32:25 <TrueBrain> michi_cc: I was fuzzing your new command code a bit, to see if I can crash the server from a client .. and I am trying to understand the code a bit
19:32:27 <TrueBrain> I am kinda failing :P
19:32:39 <TrueBrain> in Execute, there is "tile", which is from cp->tile
19:32:55 <TrueBrain> but there is also a "tile" in the arguments of commands
19:33:16 <TrueBrain> am I right to assume this means that for commands that have a "tile" parameter, "tile" is sent twice?
19:33:23 <TrueBrain> once in the CommandPacket itself, once as argument?
19:33:58 *** Wormnest has quit IRC (Ping timeout: 480 seconds)
19:34:40 <michi_cc> Let me have a look at it to jog my memory. I hope you don't expect me to remember *everything* :p
19:34:55 <TrueBrain> kinda do, but granted, you can look at the code :P
19:35:31 <TrueBrain> I think in the normal client flow, you extract the first TileIndex from the arguments and use that as cp->tile
19:35:48 <TrueBrain> SendNet, in command_func.h
19:36:32 <glx> IIRC there are some partial signature matching to go from commands to templates
19:38:27 *** tokai|noir has quit IRC (Ping timeout: 480 seconds)
19:39:03 <michi_cc> Yes, I think tile can indeed be sent twice. I don't recall if I tried to abstract that our or not, but I think you'd slowly go even more insane from the partial template matching if you try to reduce and expand args templated :)
19:39:23 <TrueBrain> in that case, we kinda have a bug :P
19:39:24 <TrueBrain> only for clients that misbehave
19:39:28 <TrueBrain> the cp->tile is checked to be in bound
19:39:34 <TrueBrain> but the tile in the args is not
19:39:37 <TrueBrain> and Cmd functions assume it is
19:39:39 <TrueBrain> guess what I did :P
19:40:54 <TrueBrain> also an observation, one about which I do not carry an opinion: if you send too few "data" for arguments, or too many, that is just okay
19:41:09 <TrueBrain> if you do too few, it seems things just get initialized to {}, and when you do too many, it seems to be ignored
19:41:30 <TrueBrain> but it doesn't crash, so shrug :P
19:41:40 <TrueBrain> but the tile thing is a bit less right :D
19:42:47 <TrueBrain> hihi, so in another place you did add code for it :D Nice, makes fixing it easier :P
19:43:45 <TrueBrain> Unsafe doesn't know "flags" :(
19:46:29 <michi_cc> Line 386 for reference.
19:47:15 <TrueBrain> InternalExecutePrepTest does validate cp->tile; so maybe before or after is a place to do the check
19:47:23 <TrueBrain> I dunno, this template stuff makes me dizzy :D
19:53:11 <TrueBrain> Unsafe is only called from scripts, so I guess that is not the place I was looking for :D
19:53:55 <TrueBrain> seems to do the trick
19:53:58 <TrueBrain> can't judge if it is a proper fix :P
19:57:56 <michi_cc> To touch network only, PostFromNet would be the spot. But maybe there isn't any harm in always doing the check.
19:58:41 <TrueBrain> I really do not know :)
19:58:48 *** WormnestAndroid has quit IRC (Ping timeout: 480 seconds)
19:59:07 <TrueBrain> with my gist it is more like it used to be, I guess .. but I see your point that this problem can only be created when arriving from network ..
20:01:59 <michi_cc> Actually, it really has to be for *all* Post.... variants. DoCommandPInternal used to have a tile check, which for whatever reason I didn't put back in.
20:02:37 <TrueBrain> well, with that check, the fuzzer doesn't crash instantly anymore .. it is now however giving timeouts .. which I do not actually understand :P
20:06:00 <TrueBrain> seems I can safely ignore that value or something .. just that some commands execute much slower than others
20:06:04 <TrueBrain> which is to be expected :P
20:07:11 *** gelignite has quit IRC (Read error: Connection reset by peer)
20:07:13 *** gelignite has joined #openttd
20:09:47 <michi_cc> The part in PostFromNet could also be moved to InternalPost, but it would usually check the identical value twice as most InternalPost calls/overloads will just pass the first arg as tile.
20:09:51 <TrueBrain> just note that InternalExecutePrepTest also does this test, similar to your second one
20:09:57 *** Wormnest has joined #openttd
20:11:12 <michi_cc> Ah yes, so if we ignore that InternalPostBefore could show an error message at an invalid tile location, the part in InternalPost can be dropped.
20:11:12 <TrueBrain> (which is executed by InternalPost already)
20:11:40 <TrueBrain> sadly, InternalPostBefore does use the tile already
20:11:51 <TrueBrain> so I guess you can remove it from InternalExecutePrepTest instead :P
20:12:16 <TrueBrain> mind if I leave it to you to make an actual PR out of this? :)
20:13:26 <TrueBrain> let me apply it and test it :)
20:13:37 <andythenorth> linkedin is offering me jobs :P
20:13:54 <andythenorth> mostly vastly inappropriate ones
20:14:05 <TrueBrain> patch fails to apply .. eeuuuhhhhh
20:15:08 <michi_cc> Eh, maybe I made a copy/paste fail.
20:15:15 <TrueBrain> I did it manually, it is fine :)
20:15:17 <michi_cc> Good commit messages:? "Fix: The tile in commands received from a client wasn't validated properly."
20:15:28 <TrueBrain> the first TileIndex ?
20:15:33 <TrueBrain> but yeah, sounds good
20:17:25 <TrueBrain> sadly, this change requires to recompile nearly everything ..... taking a bit long :D
20:17:50 *** WormnestAndroid has joined #openttd
20:18:17 <TrueBrain> michi_cc: yeah, seems to work :) Doesn't insta-crash at least :P
20:19:30 <TrueBrain> it did find some other crashes in the meantime .. lets find out what they are about :)
20:20:00 <TrueBrain> CmdAutoreplaceVehicle crashes :D
20:20:09 <TrueBrain> but that seems unrelated to your work :P
20:20:39 <TrueBrain> IsChainDepot assumes it is the first vehicle, but that is actually never validated :P
20:22:00 <TrueBrain> more commands assume to be executed on the first
20:22:07 <TrueBrain> but never actually validate :) I will make a list :P
20:22:53 <TrueBrain> (means I can crash / do weird shit on servers as client)
20:24:13 <LordAro> so who wants to file the CVE? :p
20:24:30 <TrueBrain> an excuse to get 13.0 updated in Debian? :P
20:25:23 <michi_cc> msys2 seems to want to be special today again (some timeout error in setup step).
20:27:52 <TrueBrain> okay, this is a rather efficient way of finding issues with command handling ... more efficient than I expected, I have to admit
20:28:09 <TrueBrain> I guess I should load a savegame, instead of what-ever-it-has-loaded-now
20:28:20 <TrueBrain> maybe the title screen? Dunno .. what does -vnull load?
20:28:46 <TrueBrain> title screen seems most sensible
20:29:29 <glx> -vnull doesn't load anything by default
20:29:31 <TrueBrain> 6% code coverage .. lol
20:29:54 <TrueBrain> clearly you do not NEED to do that :P
20:30:01 <TrueBrain> and "nothing" doesn't exist in the OpenTTD world
20:30:07 <TrueBrain> so it was either a flat map, or the title screen
20:30:11 <TrueBrain> I am betting on the latter
20:30:23 <TrueBrain> (given the commands do find vehicles to manipulate :P)
20:30:52 <glx> ah -vnull and nothing else ? you are in intro menu then
20:32:14 <TrueBrain> Cumulative speed : 20000 execs/sec
20:32:19 <TrueBrain> now that is a nice rounded value :D
21:10:30 *** HerzogDeXtEr has joined #openttd
21:33:22 <TrueBrain> some crashes are a bit funny ...
21:33:56 <TrueBrain> that seems to be smaller than AXIS_END
21:33:59 <TrueBrain> I do not believe this :P
21:37:27 <TrueBrain> if (!ValParamRailtype(rt) || !IsValidAxis(axis)) return CMD_ERROR; doesn't return
21:38:01 <TrueBrain> maybe somewhat related question .. why is Axis such a wide storage container :P
21:40:46 <michi_cc> Because enums with types are "new"? And no type == default int.
21:41:21 <TrueBrain> but why does this check not dismiss the command ..
21:41:41 <TrueBrain> gdb tells me the function is optimized out .. which makes sense, as it is an inline static
21:42:06 <michi_cc> With or without optimizations? No idea what the actually semantic guarantees for enums values that are not in the enum are.
21:42:20 <TrueBrain> I have a "normal" debug build
21:43:03 *** andythenorth has quit IRC (Quit: andythenorth)
21:44:14 <TrueBrain> it looks like this enum validation is not actually doing anything :P
21:46:53 <michi_cc> Does modifying it to "return (int)d < (int)AXIS_END" change anything (i.e. does this trick the compiler)?
21:47:20 <TrueBrain> haha, it seems that the signed part is the issue here :P
21:47:27 <TrueBrain> 4009754653 is -285212643
21:48:57 <TrueBrain> seems it is a terrible idea to have enums without a type for the things we use it for :D
21:49:36 <michi_cc> So basically enum Axis : byte should work, too. (Which is the assumed type in the EnumPropsT anyway).
21:49:36 <TrueBrain> yup, no longer crashes with "enum Axis : uint8 {"
21:49:57 <TrueBrain> no clue why we call it "byte" there, honestly
21:50:04 <TrueBrain> uint8 we use everywhere, not?
21:50:38 <TrueBrain> Direction and DiagDirection are correct, which are in the same file
21:54:53 <TrueBrain> at least an easy fix
21:57:58 <TrueBrain> most difficult part was thinking off the commit message
22:00:29 <TrueBrain> next one ... something with roadtype that is not going well .. hmm
22:01:21 <TrueBrain> rt = 128 .. okay ...
22:01:36 <TrueBrain> return roadtype != INVALID_ROADTYPE && ...
22:01:43 <TrueBrain> shouldn't that be <
22:02:13 <TrueBrain> so we end up in HasBit, with a << 128
22:02:16 <TrueBrain> which ofc makes it all zero
22:02:42 <TrueBrain> so not sure why it doesn't fail .. interesting ..
22:03:03 <TrueBrain> print ValParamRoadType(128) -> true
22:04:10 *** Samu has quit IRC (Quit: Leaving)
22:05:59 <TrueBrain> I do not really understand why it returns true, honestly
22:08:16 <TrueBrain> x & (uint64)(1 << 128)
22:08:24 <TrueBrain> should always return 0 .. hmm
22:08:38 <TrueBrain> too much of this code is inlined, so hard to see what it does via gdb
22:09:11 <peter1138> Might need LL there?
22:10:20 <michi_cc> "In any case, if the value of the right operand is negative or is greater or equal to the number of bits in the promoted left operand, the behavior is undefined."
22:11:00 <michi_cc> So, the compiler may pick whatever.
22:11:44 <TrueBrain> on ROADTYPE_END however ...
22:15:39 <TrueBrain> ironically, ValParamRailtype was doing the right thing :D
22:15:56 <TrueBrain> the fuzzer found over 100 uniques ways to crash because of this one boo-boo
22:16:11 <TrueBrain> (unique here is any codepath that is different)
22:22:18 <TrueBrain> it still amazes me how fuzzers manage to find these things this quickly :)
22:25:35 <TrueBrain> right, so far the remaining crashes are about FrontEngine not being validated
22:32:01 *** gelignite has quit IRC (Quit: Stay safe!)
22:51:26 <TrueBrain> CmdRemoveRoadStop with weird dimensions crashes :D
23:11:00 *** sla_ro|master has quit IRC ()
23:13:12 *** Wormnest has quit IRC (Ping timeout: 480 seconds)
23:16:59 <TrueBrain> TileAddWrap((TileIndex)19017, 2, 4294967279) says: sure, I am in bound :D
23:17:22 <TrueBrain> you can call CmdRemoveRoadStop with a negative height :D
23:17:37 <TrueBrain> there is an implicit uint -> int cast
23:17:42 <TrueBrain> took a bit of time to spot that
23:17:58 <TrueBrain> (well, I had to add some volatile variables to inspect what actually was going on :P)
23:18:18 *** WormnestAndroid has quit IRC (Ping timeout: 480 seconds)
23:19:53 <TrueBrain> this is a bug introduced during conversion I think :)
23:20:14 *** WormnestAndroid has joined #openttd
23:25:14 <TrueBrain> right, enough for today .. tomorrow the other two issues it found :P
23:26:04 *** HerzogDeXtEr has quit IRC (Read error: Connection reset by peer)
23:38:43 *** WormnestAndroid has quit IRC (Read error: Connection reset by peer)
23:38:57 *** WormnestAndroid has joined #openttd
23:40:22 *** Wormnest has joined #openttd
23:44:44 *** WormnestAndroid has quit IRC (Read error: Connection reset by peer)
23:45:04 *** WormnestAndroid has joined #openttd
continue to next day ⏵
