Times are UTC Toggle Colours
00:39:51 *** Montana has quit IRC 02:13:55 *** D-HUND has joined #openttd 02:17:22 *** debdog has quit IRC 02:18:09 *** D-HUND is now known as debdog 03:05:25 *** glx has quit IRC 06:01:13 *** andythenorth has joined #openttd 06:21:15 *** Flygon has joined #openttd 06:24:49 *** sla_ro|master has joined #openttd 06:35:46 *** andythenorth has quit IRC 06:40:57 *** andythenorth has joined #openttd 07:16:43 *** Tirili has quit IRC 07:18:08 *** urdh has quit IRC 07:18:20 *** urdh has joined #openttd 07:21:33 *** andythenorth has quit IRC 07:40:59 *** andythenorth has joined #openttd 07:56:45 *** andythenorth has quit IRC 07:57:17 *** andythenorth has joined #openttd 08:03:58 *** andythenorth has joined #openttd 08:23:20 *** Etua has joined #openttd 08:28:56 *** Etua has quit IRC 08:29:18 *** Etua has joined #openttd 08:33:03 *** Etua has quit IRC 09:11:56 *** Samu has joined #openttd 09:26:24 *** Etua has joined #openttd 09:52:44 *** Etua has quit IRC 11:07:46 *** WormnestAndroid has quit IRC 11:07:49 *** WormnestAndroid has joined #openttd 11:18:33 *** gelignite has joined #openttd 11:44:25 *** Smedles has quit IRC 11:44:35 *** Smedles has joined #openttd 12:18:28 *** OsteHovel has joined #openttd 12:19:27 *** glx has joined #openttd 12:19:27 *** ChanServ sets mode: +v glx 12:54:47 *** peignenappe has joined #openttd 12:55:47 *** peignenappe has quit IRC 13:09:41 *** nielsm has joined #openttd 13:25:47 *** nielsm has quit IRC 13:49:09 *** Flygon has quit IRC 13:50:15 *** Flygon has joined #openttd 14:26:27 *** andythenorth has quit IRC 15:01:07 *** Wormnest has joined #openttd 15:40:54 *** andythenorth has joined #openttd 15:45:41 <DorpsGek> [OpenTTD/OpenTTD] 2TallTyler updated pull request #9931: Feature: Multi-track level crossings https://github.com/OpenTTD/OpenTTD/pull/9931 15:48:24 <supermop_work> yo 16:03:38 *** HerzogDeXtEr has joined #openttd 16:33:01 *** Smedles has quit IRC 16:33:34 *** Smedles has joined #openttd 16:46:29 <DorpsGek> [OpenTTD/OpenTTD] 2TallTyler updated pull request #9931: Feature: Multi-track level crossings https://github.com/OpenTTD/OpenTTD/pull/9931 16:50:22 <DorpsGek> [OpenTTD/OpenTTD] 2TallTyler commented on pull request #9931: Feature: Multi-track level crossings https://github.com/OpenTTD/OpenTTD/pull/9931#issuecomment-1179188035 16:54:06 *** HerzogDeXtEr has quit IRC 16:55:09 *** Flygon has quit IRC 17:34:28 *** andythenorth has quit IRC 17:39:13 <Samu> it's official, now I know I need directions "per segment" based 17:40:00 <Samu> i finally have proof 17:44:48 <Samu> tile in question, tile 945 https://i.imgur.com/upim1JY.png 17:46:55 <Samu> bad screenshot, some hidden text 17:47:52 *** WormnestAndroid has quit IRC 17:48:17 <Samu> https://i.imgur.com/CzmlktV.png the line that says "New entry! make sure we don't check it again, cur_tile 945 with direction 64", is added to the closed list 17:48:54 <Samu> then on the bottom, before the last yellow line, it detects that tile is already on the list 17:49:00 <Samu> with that direction 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:54:24 <Samu> a int has only 64 bits 17:55:11 <Samu> and i need to place segmentdirs combined with tile dirs somehow 17:55:20 <Samu> I'm not sure what to do 17:55:37 <Samu> how to go about that 18:17:23 *** Wormnest has quit IRC 18:23:17 *** afon has joined #openttd 18:23:19 *** WormnestAndroid has joined #openttd 18:24:46 *** WormnestAndroid has quit IRC 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:00:09 <LordAro> /p[12]//g 19:00:15 <LordAro> problem solved 19:00:19 <TrueBrain> I dare you 19:00:54 <LordAro> hehe 19:01:13 <TrueBrain> :D 19:10:54 <Samu> how do i copy the contents of a table into another? tried looking here but I don't see anything specific http://www.squirrel-lang.org/squirreldoc/reference/language/builtin_functions.html#table 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:23:34 <glx> http://squirrel-lang.org/doc/squirrel2.html#d0e1264 19:25:41 <Samu> exp:= 'clone' exp, why is this example so difficult to understand 19:25:52 <glx> it's not an example 19:25:59 <glx> it's the parser syntax 19:26:46 <glx> something like "local copy = clone original;" should work 19:27:08 <Samu> ah, gonna try 19:27:12 <Samu> thx 19:28:17 <andythenorth> yo 19:31:40 *** tokai has joined #openttd 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 19:34:21 <glx> templates magic 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:16 <TrueBrain> https://github.com/OpenTTD/OpenTTD/blob/master/src/command_func.h#L240, to make it a bit easier to follow what I mean :P 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 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:17 <TrueBrain> okay 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:40 <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:38 <michi_cc> So basically Unsafe needs something like https://github.com/OpenTTD/OpenTTD/blob/master/src/command_func.h#L152, too. 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:53 <TrueBrain> https://gist.github.com/TrueBrain/463f92f6220d535f582f52dcae08e464 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 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 20:07:13 *** gelignite has joined #openttd 20:08:53 <michi_cc> TrueBrain: An exact replica of the pre-template code would be: https://gist.github.com/michicc/fb2e89df61c1d1a723ac70364bef9403 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:12 <michi_cc> Gist updated. 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:15 <DorpsGek> [OpenTTD/OpenTTD] michicc opened pull request #9942: Fix: The first tile in commands received from a client wasn't validated properly. https://github.com/OpenTTD/OpenTTD/pull/9942 20:18:17 <TrueBrain> michi_cc: yeah, seems to work :) Doesn't insta-crash at least :P 20:18:39 <DorpsGek> [OpenTTD/OpenTTD] TrueBrain approved pull request #9942: Fix: The first tile in commands received from a client wasn't validated properly. https://github.com/OpenTTD/OpenTTD/pull/9942#pullrequestreview-1033348320 20:18:44 <TrueBrain> tnx for the fix! 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:39 <glx> you need to specify -g 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 20:37:45 <DorpsGek> [OpenTTD/OpenTTD] michicc merged pull request #9942: Fix: The first tile in commands received from a client wasn't validated properly. https://github.com/OpenTTD/OpenTTD/pull/9942 21:10:30 *** HerzogDeXtEr has joined #openttd 21:28:22 *** afon has left #openttd 21:33:22 <TrueBrain> some crashes are a bit funny ... 21:33:26 <TrueBrain> axis=4009754653 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:37:30 <TrueBrain> eeeeeuuuuhhhhhh 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:14 <TrueBrain> funny 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 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:47:31 <TrueBrain> so ........ 21:47:58 <TrueBrain> https://godbolt.org/z/aMvTKaTqb 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:50:40 <TrueBrain> silly :) 21:54:53 <TrueBrain> at least an easy fix 21:56:58 <peter1138> enum Axis : uint1 21:57:54 <DorpsGek> [OpenTTD/OpenTTD] TrueBrain opened pull request #9943: Fix: commands with Axis in their arguments were not validated properly https://github.com/OpenTTD/OpenTTD/pull/9943 21:57:58 <TrueBrain> most difficult part was thinking off the commit message 21:58:12 <DorpsGek> [OpenTTD/OpenTTD] michicc approved pull request #9943: Fix: commands with Axis in their arguments were not validated properly https://github.com/OpenTTD/OpenTTD/pull/9943#pullrequestreview-1033412115 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:38 <TrueBrain> hmm ... 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:03:04 <TrueBrain> lol 22:04:10 *** Samu has quit IRC 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:09:38 <TrueBrain> still 0, not? 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:10:29 <TrueBrain> owh 22:10:30 <TrueBrain> okay 22:11:00 <michi_cc> So, the compiler may pick whatever. 22:11:31 <TrueBrain> https://github.com/OpenTTD/OpenTTD/blob/master/src/road.cpp#L144 <- guess changing that != into < would do the trick .. but not really nice to do that on an INVALID 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:15:57 <TrueBrain> :D 22:16:11 <TrueBrain> (unique here is any codepath that is different) 22:20:37 <DorpsGek> [OpenTTD/OpenTTD] TrueBrain opened pull request #9944: Fix: commands with a RoadType in their arguments were not validated properly https://github.com/OpenTTD/OpenTTD/pull/9944 22:22:18 <TrueBrain> it still amazes me how fuzzers manage to find these things this quickly :) 22:22:53 <DorpsGek> [OpenTTD/OpenTTD] michicc approved pull request #9944: Fix: commands with a RoadType in their arguments were not validated properly https://github.com/OpenTTD/OpenTTD/pull/9944#pullrequestreview-1033422877 22:25:35 <TrueBrain> right, so far the remaining crashes are about FrontEngine not being validated 22:32:01 *** gelignite has quit IRC 22:51:26 <TrueBrain> CmdRemoveRoadStop with weird dimensions crashes :D 22:51:30 <TrueBrain> this is fun :P 23:11:00 *** sla_ro|master has quit IRC 23:13:12 *** Wormnest has quit IRC 23:16:59 <TrueBrain> TileAddWrap((TileIndex)19017, 2, 4294967279) says: sure, I am in bound :D 23:17:00 <TrueBrain> awesome :) 23:17:14 <TrueBrain> 4294967279 is -17 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:39 <TrueBrain> lovely :) 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 23:19:53 <TrueBrain> this is a bug introduced during conversion I think :) 23:20:14 *** WormnestAndroid has joined #openttd 23:25:02 <DorpsGek> [OpenTTD/OpenTTD] TrueBrain opened pull request #9945: Fix: CmdRemoveRoadStop didn't validate the height property properly https://github.com/OpenTTD/OpenTTD/pull/9945 23:25:14 <TrueBrain> right, enough for today .. tomorrow the other two issues it found :P 23:26:04 *** HerzogDeXtEr has quit IRC 23:38:43 *** WormnestAndroid has quit IRC 23:38:57 *** WormnestAndroid has joined #openttd 23:40:22 *** Wormnest has joined #openttd 23:44:44 *** WormnestAndroid has quit IRC 23:45:04 *** WormnestAndroid has joined #openttd