Time Nick Message 00:38 rubenwardy > this issue should be assumed to be a remotely-exploitable security vulnerability on the server 00:38 rubenwardy if this is actually true, then you should probably be disclosing using the security policy 01:02 erlehmann rubenwardy well for the previous UB i was asked to file issues after posting traces here. 01:03 erlehmann rubenwardy but i can mail you and celeron55 future issues of that category 01:04 erlehmann i.e. “assume its exploitable until you know it is not” 01:04 erlehmann we discussed the noise thing already though 01:05 erlehmann i think *all* UB is a security issue, yes, even fast inverse square root 01:06 erlehmann rubenwardy, don't worry, when i have actual exploits i will not post them on the bug tracker 01:06 erlehmann so far the best i can do is crash servers, but a bunch of people can do that 01:07 erlehmann in fact, it was weaponized already in the past (“if you attack my base, i crash the server” hehe) 01:07 MTDiscord erlehemann, I don't want to get in your way or take up your time, but if there's anything I can do to help with this work I'm up for it. I'm interested in security, and I am looking for ways to get involved with Minetest development. 01:08 erlehmann josiah_wi, read up about ubsan, asan, tsan. figure out the ways how UB is done. for the noise thing i think pgimeno had a patch posted here? 01:09 erlehmann josiah_wi, start here https://blog.regehr.org/archives/213 and here https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html 01:09 MTDiscord Will do. Thanks for giving me a good direction to start in. 01:10 erlehmann josiah_wi most of these are probably easily fixable 01:11 erlehmann josiah_wi i think the best thing you could do if you want to invest some work would be to make --random-input predictable based on seed and then compile minetest with some sanitizers like ubsan turned on a CI and let it play randomly for 5 minutes. 01:11 erlehmann it will prob find a bunch of bugs 01:13 erlehmann josiah_wi i retract my assertion. the best thing you could do is fixing the bugs belonging to the stack traces i posted ;) 01:14 erlehmann i haven't posted the heavy ones so far 01:14 erlehmann bc i have not understood 01:14 erlehmann by heavy i mean big oof 01:16 MTDiscord Ok. I'll probably need to keep a document with all the stack traces you have finished the sluething on so I can keep track of them. I'll do the assigned reading first though, so I'm familiar with what's going on. 01:16 erlehmann well i make issues for them 01:18 erlehmann josiah_wi do not worry about getting in my way or taking my time. i want to do as little as possible. am very lazy. 01:18 erlehmann also low energy 01:18 erlehmann so if you do stuff it is good! 01:18 erlehmann :) 01:19 erlehmann also note that -DCMAKE_CXX_FLAGS= for cmake sets compiler flags, i learned that from sfan5 01:24 MTDiscord Good reminder. I'm reading the undefined behavior guide, and I just wanted to share this: "Similarly, there have been C compilers that optionally give undefined semantics to unsigned overflow to speed up other loops." Not the first time compilers have blown my mind, but it's always just as awe inspiring every time. 01:29 erlehmann josiah_wi you can use https://godbolt.org/ to see what compilers can do 01:32 erlehmann josiah_wi check out this example: https://godbolt.org/z/P3EY4TrWP 01:36 v-rob Honestly, I doubt it would be a security vulnerability in practical cases. All common modern processors use two's complement, and even the C++17 standard guarantees two's complement on signed overflow instead of UB. We're on C++11, of course, but I doubt most compilers will do really weird things on signed overflow. Fixing it would probably be good, but not critical. 01:41 erlehmann v-rob it is a case of “better safe than sorry” 01:41 erlehmann i did not know the C++17 thing it sounds neat 01:42 erlehmann don't worry, if i have an actual exploit, i will call it “client sent server mods version 2” or so 01:42 erlehmann not something boring 01:44 erlehmann v-rob quoting https://blog.regehr.org/archives/213 01:44 erlehmann > It is very common for people to say — or at least think — something like this: 01:44 erlehmann >> The x86 ADD instruction is used to implement C’s signed add operation, and it has two’s complement behavior when the result overflows. I’m developing for an x86 platform, so I should be able to expect two’s complement semantics when 32-bit signed integers overflow. 01:44 erlehmann > THIS IS WRONG. You are saying something like this: 01:44 erlehmann >> Somebody once told me that in basketball you can’t hold the ball and run. I got a basketball and tried it and it worked just fine. He obviously didn’t understand basketball. 01:45 erlehmann v-rob obviously nothing bad happens if you hold the ball and run over the field, but you definitely aren't playing basketball at that point. 01:45 erlehmann damn 01:46 MTDiscord Are we really getting pedantic about a fucking game engine that holds no data of important 01:46 erlehmann Jordach “the compiler does not need to emit code to format your disk. Rather, following the OOB array access your computer will begin executing exploit code, and that code is what will format your disk.” 01:47 erlehmann so, yes 01:49 erlehmann also there is the philosophical issue of programs with unconditional UB having no meaning 01:51 MTDiscord Jordach, do I hear you volunteering to be the test subject for any exploits discovered? Especially remote code execution. 01:52 erlehmann to be fair, most of the stuff found will probably not result in remote code execution 01:52 erlehmann sadly 01:52 erlehmann but i bet there are some hidden gems in minetest 01:54 MTDiscord Wow. That example of a switch with UB is impressive. For the record, I learned to never, ever write a switch statement without including a default case... 01:56 erlehmann well the thing is that this function shows why UB is useful 01:56 erlehmann you can make your function much faster if you assume that the function will only be called in a correct way 01:56 erlehmann but then it breaks horribly if it is not called that way 01:58 MTDiscord Heh, I just tried that switch statement, and if you delete the first or last case, it produced exactly the same code :-D 01:58 MTDiscord Indeed. But like you pointed out you're making that assumption. And actually, you could just make that behavior explicit and write a comment explaining why you sacrificed the sanity of your codebase for that extra nanosecond of speed. 01:59 MTDiscord Warr, if you add a default case does it add an extra check, or do all the checks? I can't edit it on mobile for some reason. 02:01 MTDiscord Hmm, that's surprisingly complicated. It still unconditionally does the "sub" that corresponds to the optimized implementation, but it also now adds a compare to decide whether to use the default case afterwards. 02:02 MTDiscord Ah, that's what I'd hoped it would do. Interesting. 02:03 erlehmann and because it is “surprisingly complicated” to figure out what it does, one needs to be careful 02:03 erlehmann else you get unwelcome surprises 02:03 erlehmann also try switching between optimization levels and compilers 02:04 MTDiscord If I remember right from the reading erlehmann gave me, modifying a volatile isn't considered a side effect in some compilers? 02:04 MTDiscord yeah, this just looks wrong 02:05 MTDiscord because it's being passed an int, but it produces the same behavior as if it were passed an unsigned int 02:05 MTDiscord and I have cases for 0, 1, 2, and the default, but it looks like it's not accounting for the possibility that the input could be negative...? 02:05 MTDiscord I don't remember the nuances of C that much tho 02:06 MTDiscord heh, I added a case for -5 and it generated a LUT instead. 02:07 MTDiscord Now we've suddenly gone from integer overflow risk to out-of-bounds memory access risk 02:08 MTDiscord Wait seriously? Despite the default case? 02:08 erlehmann v-rob please look at the logs 02:08 erlehmann v-rob i answered you 02:09 MTDiscord https://godbolt.org/z/senbP5v64 02:11 v-rob Yeah, I agree it's better to be safe than sorry, and we certainly should fix it, just saying that it's probably not a security issue in practicality. 02:13 erlehmann how can you be so sure? 02:17 v-rob Because all the platforms anybody in their right mind will try to compile Minetest on don't have an issue with it, x86, ARM, etc. I can't be _certain_ (I mean, a compiler might have a bug which causes a vulnerability in the final executable -- we can never be _sure_), but I can be pretty darn close to it. 02:17 v-rob And that "pretty darn sure" means that fixing it is not necessarily critical. 02:18 v-rob That's why I said "in practicality". In theory, anything might happen, but practically, it won't. 02:30 MTDiscord @Warr1024 there's some serious haxxory going on with that assembly. Did you test whether you can observe UB? It's doing a 32-bit subtraction followed by a special conditional move. I can't tell whether it detects negative values or not. 02:31 MTDiscord No, I didn't test it, can barely read it and not sure if my interpretation is correct. I haven't looked at x86 assembly since the 486 days. 02:37 erlehmann v-rob, what is the output of the following program according to your interpretation? 02:37 erlehmann #include 02:37 erlehmann #include 02:37 erlehmann int main(int argc, char** argv) { int i = 1; if (i + INT_MAX < i) { printf("%i\n" ,i + INT_MAX);} } 02:41 erlehmann (this is, of course, a trick question) 02:43 erlehmann i mean, -2147483648 is pretty much lower than 1 right? 02:44 erlehmann the fun thing is, it even gives different results depending on if i compile it with ubsan or not 02:47 v-rob With my compiler, the printf will not be reached, which is, of course, valid. However, it creates no security hazard. That's my point: UB like signed integer overflow can lead to bugs, but signed integer overflow will not create a security hazard in any practical situation Minetest will every be in. 02:48 erlehmann well if the statement is not reached, there surely is no 2s complement 02:49 erlehmann and i already said that i doubt many of those will be exploitable, but in general, who knows 02:58 MTDiscord for every hack made for speed 02:58 MTDiscord a problem is introduced 09:55 erlehmann josiah_wi maybe try to reproduce this and see if you get similar results https://github.com/minetest/minetest/issues/11624 12:07 pgimeno erlehmann: what noise patch? 12:08 erlehmann for the mapgen thing 12:08 pgimeno not sure what you mean 12:08 pgimeno oh the seed 12:08 erlehmann pgimeno, maybe i confused you sorry 12:08 erlehmann yeah 12:09 erlehmann you fixed that? 12:09 pgimeno yeah, it is a 1 character fix, just add a 'U' to the last constant in the line that triggers the UB 12:10 pgimeno sfan is aware I believe 12:11 pgimeno anyway, since it seems we expect wrapping behaviour, why don't we just add -fwrapv to the command line and get done with all these "bugs"? 12:13 pgimeno erlehmann: with -fwrapv in your example, I get -2147483648 printed 12:14 erlehmann pgimeno that is because then you are no longer playing basketball 12:14 pgimeno erlehmann: I know, we're playing GNU basketball 12:14 pgimeno that's not necessarily a bad thing, the NBA has its own rules for example 12:14 erlehmann the problem with -fwrapv is that it hides the fact that it was probably not intended to happen 12:15 erlehmann you want discontinuities in the noise? i have giant stone cubes in the sky for you. 12:15 pgimeno not intended for signed and intended for unsigned? hmm, I don't follow that logic 12:15 erlehmann signed overflow is very rarely intended actually. if so, projects use -fwrapv from the start. 12:17 pgimeno this is a collaborative project, the collaborators don't mandate command line arguments and often are not aware about it being UB 12:17 erlehmann i.e. in minetest, i doubt it is intended at all 12:17 pgimeno for the seed it may well be intended 12:17 pgimeno it's just a hash 12:17 erlehmann but that's the point, if you are not aware of it then the programs wrapping behaviour is something you probably do not intend. 12:17 pgimeno but the same can be said about unsigned overflow, or not? 12:18 erlehmann less likely, but true. i have not investigated it yet. 12:18 erlehmann in general, saying “just do whatever” will hide logic bugs. 12:19 erlehmann i mean, consider a length field overflowing somehow. you probably want a stacktrace then, not wrapping behaviour. 12:21 erlehmann pgimeno, for the seed the ideal thing would be something that is as close to what the previous thing was compiled to on x86 / x64 / arm 12:21 erlehmann i mean you tried to approximate that, right? 12:22 pgimeno yeah, and the 'U' solution is equivalent 12:23 pgimeno I tried with a loop over all values of the seed, the result was the same in all cases 12:25 sfan5 if you build something as close to the previous code isn't that an admission that the wrapping sign behaviour was intended? 12:28 erlehmann sfan5 no, in this case it is a compatibility aid, because it is a seed. but in alle cases through out the entire code base? i strongly doubt that every signed integer overflow was intended, specifically because some commit messages show that this code has already been refactored to *avoid* UB. 12:28 erlehmann rated E for effort 12:33 erlehmann sfan5, are the data races between threads that occur in minetest also intended? bc surely the best way to fix those races is to work out which thread wins most of the time and take that as established behaviour. 12:34 MTDiscord erlehmann, rebuilding with different sanitize flags may take some time. Do you want me to start with the memory issue you linked? 12:34 erlehmann (if compatibility is more important than whatever else may affect the decision) 12:35 erlehmann josiah_wi decide for yourself. but it is a goo issue, if you can debug and fix that, it would actually enable anyone to get more memory issues – as the application exits once it encounters this one. 12:35 erlehmann a goo issue → a good issue 12:36 MTDiscord Righto. Will start with that one! 12:36 erlehmann in general, i expect most of the issues that turn up once enough people try the sanitizer stuff to be easily fixable 12:38 erlehmann so far i have encountered: wrong variable widths/types, uninitialized fields, data races between threads, having a null pointer where it is illegal, use after free, pointers appearing to point to objects that are not of the correct type, … 12:38 erlehmann except for the data races, fixing that stuff is pretty straight-forward 12:40 MTDiscord erhlemann, which compiler should I use? I typically use GCC, but maybe it doesn't have the memory option? 12:41 erlehmann ah, not all compilers have all sanitizers 12:41 erlehmann sorry 12:41 erlehmann compile with clang++ 12:42 erlehmann to my knowledge only gcc and clang have sanitizers, and both compilers have some useful options the other does not have 12:42 erlehmann but only clang++ had the memory sanitizer on my machine 12:42 MTDiscord Good to know. 12:42 erlehmann also clang & gcc differ on the optimizer 12:43 erlehmann so if you have UB they might just disagree on what the assembly is 12:46 MTDiscord I know people have their own personal style of each cmake command that they're used to, but you don't need quotes around every -D option; could save some typing. There's also a CMAKE_BUILD_TYPE variable. It can be set to Release, Debug, or RelWithDebug (specific to Minetest) IIRC and will add appropriate optimization flags. 12:49 erlehmann “you don't need quotes around every -D option” actually, i do. 12:50 MTDiscord Interesting. Which shell? 12:50 erlehmann some shells, like the rc(1) shell that i use, choke on unquoted equals signs there 12:50 erlehmann it also makes it obvious what value is given 13:05 pgimeno this is what I meant about the danger of bools: https://godbolt.org/z/zvPsz6Pr5 13:06 pgimeno clang loves to do arithmetic with them 13:07 erlehmann pgimeno, well the boole can only be 0 or 1, so it is *perfectly legal* 13:08 pgimeno of course, my point is that a bool out of range can result in a buffer overflow 13:08 erlehmann volatile x, or in german “bitte nicht schubsen, diese variable ist voller teile” 13:09 erlehmann pgimeno yeah, surprising isn't it? 13:09 erlehmann pgimeno and all it takes is the bool not being initialized, falling back to whatever was on the heap or wherever you are storing your precious bools nowadays 13:10 pgimeno I was emphasizing this point: 13:10 pgimeno [0911 19:13:20] so I'd say a bool taking an illegal value is a serious issue 13:29 erlehmann pgimeno, now only people like v-rob and SmallJoker have to see it that way! 13:29 erlehmann josiah_wi managed to reproduce the memory error? 13:36 sfan5 instead of spending lots of time convincing people of a certain severity better just submit a PR with a fix 13:43 erlehmann sfan5 i told you i am not exactly good at fixing 13:49 pgimeno erlehmann: first rule of free software: that who proposes, must be prepared to do the work 13:53 pgimeno problem is, with the incomplete traces it's hard to follow the tracks to the point where the bug was triggered 13:53 MTDiscord erlehmann, it is still compiling. 13:54 pgimeno if it can be accompanied by a reproduction recipe that would help me tracking the issue 13:54 pgimeno erlehmann: have you reported this one on GH? https://mister-muffin.de/p/fSgo.txt 13:57 MTDiscord Aha, I can repro. 13:59 erlehmann i have reported a new one and even made a mod to reproduce it https://github.com/minetest/minetest/issues/11625 14:00 erlehmann > lottapixel.jpg is a 4856 byte JPEG file that has been edited to say its dimensions are 64250×64250. 14:01 erlehmann i guess i should just get a better computer with more memory 14:01 erlehmann spoiler: i have no idea how to fix this! 14:02 sfan5 no backtrace? 14:02 erlehmann no, it just craps out and i did not start it with gdb 14:02 erlehmann there was one error message 14:03 sfan5 it's probably an Irrlicht bug regardless 14:04 erlehmann > 2021-09-14 15:41:34: ERROR[Main]: Irrlicht: JPEG FATAL ERROR in _tempreadfile: Corrupt JPEG data: bad Huffman code 14:05 erlehmann bug-wise, do you prefer bugs where the server or where the client crashes? 14:06 erlehmann i know how to fix it 14:07 erlehmann validate all input files against a grammar 14:08 pgimeno then you have no excuse to submit a fix if you care about the bug 14:08 MTDiscord Is there a macro that's literally just an underscore? 14:09 sfan5 possibly 14:09 pgimeno most likely translation stuff 14:09 MTDiscord make_pair("name", _("some string")) 14:10 MTDiscord Ok, I'm not even going to ask. breathes deeply 14:21 pgimeno @josiah_wi see https://www.gnu.org/software/gettext/manual/html_node/Overview.html second box 14:26 pgimeno heh guess what, gimp opens the image in #11625 (it takes a while though) 14:26 ShadowBot https://github.com/minetest/minetest/issues/11625 -- Pixelflood attack: Sending lottapixel.jpg from server segfaults client upon connection 14:32 pgimeno what's a reasonable resolution limit for images? like, 4096x4096? or is even that too much? 14:33 MTDiscord skyboxes use the largest images that i know about 14:36 MTDiscord I am missing something. The second insert into the std::map of options always causes the memory error 14:36 MTDiscord Doesn't matter which insert. It's always the second one. 14:37 MTDiscord Is it possible that std::map is responsible!? 14:41 pgimeno @josiah_wi which issue are you investigating? 14:42 MTDiscord The uninitialized value on startup. 14:42 pgimeno what issue # ? 14:44 MTDiscord 11624 14:45 MTDiscord Hah, doing all the options in a single insert avoids the error, 14:45 MTDiscord Unbelievable. 14:46 pgimeno yeah it's possible that the STL has these things, that's why valgrind has exclusion files 14:48 pgimeno https://stackoverflow.com/questions/20617788/using-memory-sanitizer-with-libstdc 14:48 pgimeno are you using libc++? 14:48 MTDiscord Frankly, making a single insert call should reduce overhead by a ridiculously small smidgen which is always nice, and looks nice and clean. OK if I make a PR? 14:49 MTDiscord I probably am using libstdc++ but I don't really know. 14:50 pgimeno do you have libc++ installed at all? 14:51 sfan5 please don't make a PR with pointless changes 14:52 MTDiscord Yes, I do. 14:52 sfan5 and going by that SO post just having libc++ installed won't do it either 14:52 sfan5 (because whatever version your distro ships will surely not be instrumented for MSan) 14:54 MTDiscord Nevermind, error came back. 14:57 MTDiscord erlehmann, I'm done with this particular issue. I don't believe there's any local fix for this issue, and there are probably hundreds like it. 15:51 appguru We don't use Travis CI anymore, right? 15:54 sfan5 no and even if we wouldn't be affected 15:55 sfan5 the reason you're asking is https://twitter.com/peter_szilagyi/status/1437646118700175360 right? 15:58 nrz yeah, didn't linked it because we moved long time ago because travis was... s**t ? 17:04 MTDiscord UbSan doesn't work if Irrlicht is dynamically linked due to https://stackoverflow.com/questions/57294792/c-ubsan-produces-false-positives-with-derived-objects. 17:08 MTDiscord Now I have to wait half an hour again to recompile with static Irrlicht. On the upside, the false positive was on a line that does have UB. It accesses a method on a pointer returned by Irrlicht, which can be null. 18:20 erlehmann could you put https://irc.minetest.net/minetest-dev in the topic? 18:21 erlehmann pls 18:22 sfan5 why 18:23 erlehmann bc kinda relevant to the channel if someone has not been there for a moment or a while or a long time 18:23 erlehmann like me rn 18:24 erlehmann it's not like the channel topic is rare real estate or that it is irrelevant 18:24 erlehmann > what's a reasonable resolution limit for images? like, 4096x4096? or is even that too much? 18:24 erlehmann pgimeno, i think the image should not be processed regardless of resolution 18:24 erlehmann it is obviously illegal 18:25 erlehmann the only way to be sure is to first check if this is a legal file according to your expectations 18:26 erlehmann which would include max resolution, but also basic sanity checking like “does this thing contain all the pixels it says it does” 18:27 erlehmann erlehmann, I'm done with this particular issue. I don't believe there's any local fix for this issue, and there are probably hundreds like it. 18:27 erlehmann josiah_wi have you found what exactly is the uninitialized memory that is used there? 18:27 erlehmann bc that stack trace looks like a mess 18:28 erlehmann josiah_wi do -fno-sanitize=vptr 18:28 erlehmann josiah_wi that will silence all the vptr warnings 18:38 sfan5 merging #10796, #11618 in math.ceil(math.pi*2) minutes 18:38 ShadowBot https://github.com/minetest/minetest/issues/10796 -- Chop game background by appgurueu 18:38 ShadowBot https://github.com/minetest/minetest/issues/11618 -- Add Windows-specific CreateTempFile() implementation by sfan5 18:51 erlehmann sfan5, is there still time to test https://github.com/minetest/minetest/pull/10796 ? or why are you posting this here 18:51 erlehmann oh it is merged 18:53 erlehmann can i suggest more stuff to delete if it is not used 18:53 erlehmann like provably not used 18:53 erlehmann i.e. dead code 18:59 sfan5 you can 19:00 erlehmann sfan5, do you have any kind of test case for the jpeg loader function? i ask becauses of https://github.com/minetest/irrlicht/commit/594de9915346a87f67cd94e28c7933993efb5d3b 19:00 sfan5 no 19:00 erlehmann if you give me a thing that loads jpegs, a cli program, i can instrument it and find more jpeg loader bugs 19:00 erlehmann alternatively, i'd have to write one myself, but the downside is that i'd have to write one myself obv 19:01 sfan5 the smallest possible irrlicht using program is pretty small so that should be quick 19:01 erlehmann i actually have done jpeg fuzzing for work years back 19:01 erlehmann was pretty funny 19:02 erlehmann the result was that sometimes a bitflip is enough to totally ruin someones day (if that day hinges on loading a jpeg) 19:04 erlehmann sfan5, neat that you fixed it fast btw 19:04 erlehmann sfan5 i hope my mod was a good way to test it. or could i have made it any easier? 19:10 sfan5 was easy enough 19:21 pgimeno alternatively, i'd have to write one myself, but the downside is that i'd have to write one myself obv <-- if you care enough, do the work yourself; if you don't, don't ask others to do it for you 19:24 erlehmann pgimeno, i asked if there was one available, not to make one available 19:25 erlehmann “if you give me X, i can do Y” is true in the most naive form, i.e. it is not a request to *create* X 19:26 erlehmann please do not thing i am expecting anything i am not clearly saying 19:26 erlehmann think 19:26 erlehmann sorry 19:27 erlehmann pgimeno i do care, but like anyone elses i have limited time and energy. and since minetest scandalously broke my just released pixelflood mod i have to find a new malformed image on which it chokes to preserve the crash functionality, i.e. re-enable spacebar heating! :P 19:28 sfan5 you said the TGA code is "battle-tested" but I am sorry to inform you that such a bug is in there too 19:28 sfan5 so you could look for that 19:30 erlehmann sfan5, battle-tested means exactly that, it has survived so far without major breakage. i will try to break everything, regardless if i like it or not. 19:31 erlehmann i.e. all image formats will eventually fall 19:31 erlehmann (and be fixed) 19:32 erlehmann btw i found that “server owners should make sure they have no corrupted files” comment hilarious 19:41 MTDiscord erlehmann, the uninitialized memory is completely the fault of the std::map implementation, apparently. 19:41 erlehmann josiah_wi can you not trick it into initializing something? 19:41 erlehmann i mean, that is unfortunate to begin with 19:42 erlehmann bc memory sanitizer will, as i understand it, only complain if this can actually affect program flow 19:42 erlehmann so ig it matters in *some* arcane way i do not understand 19:43 erlehmann josiah_wi statically recompiling is prob smarter than what i did with ignoring vptr 19:46 MTDiscord The static lib either didn't fix it, or I messed some up. 19:48 MTDiscord I tried to trick std::map by making a single insert() call, but that failed too. 20:12 pgimeno thing is, the C/C++ libraries are allowed to invoke UB because they are aware of the 8nternals of the compiler they're designed to work with, so you may have false positives there 20:12 pgimeno internals* 20:14 erlehmann pgimeno, “allowed to invoke UB”? 20:14 erlehmann that would be news to me 20:14 erlehmann pgimeno do you have any further reading on that? 20:15 erlehmann josiah_wi did you try -fno-sanitize=vptr though? 20:15 specing UB is a way of life for C/C++-heads 20:15 specing they can't sleep well at night unless they have at least one serious memory bug per 100 lines of code 20:16 MTDiscord Holy shit, this Targagate thread on GitHub ... what's all this talk about breaking old worlds? 20:17 MTDiscord I have to be crazy because there's all this debate but the solution for MCL* to guard against future deprecation is obvious. 20:17 MTDiscord Just read the maps from TGA, then write them to TGA (for storage) and to PNG (for the clients to display) 20:17 MTDiscord Is there something I missed about why that wouldn't work? 20:18 sfan5 it works perfectly well 20:18 sfan5 MCL does not even need TGA for storage 20:18 sfan5 the maps are only ever written 20:18 pgimeno yeah, old servers with new clients @Warr1024 20:19 MTDiscord Server owners are probably just going to have to maintain their servers 20:20 MTDiscord Well, we're still talking about deprecation, not removal. Presumably we would flag TGA deprecated in 5.5, and server owners would have until 5.6 is released to handle that. 20:21 sfan5 well I don't think we've ever had "new clients may not fully work on previous version servers" before 20:21 MTDiscord Or maybe 5.6 would end up being called 6.0 instead, if people are going to make such a stink about one file format. 20:22 MTDiscord I see no reason why we have to assume 6.0 needs to be some huge milestone. Just trimming the fat out of Irrlicht could be sufficient justification. It's really just a number... 20:23 erlehmann “Presumably we would flag TGA deprecated in 5.5, and server owners would have until 5.6 is released to handle that.” – look, if debian is still on 5.3 while you release 5.5, no server owner will even know. 20:23 sfan5 debian's lousy update policy will not dictate development. 20:23 MTDiscord I can tell you from experience that being on Debian means you use FlatPak :-D 20:24 MTDiscord Well, for client use, anyway. Servers are more a Docker thing anyway. 20:25 erlehmann as i see it, the choice is: break stuff or do not break stuff. it's not like the TGA code is hampering anyone's ability to refactor anything, or is it?? 20:25 erlehmann btw, i would care much less if the attitude “haha i break it now someone else fix it” was verboten 20:27 MTDiscord Actually, what I was hoping for was to do the fix first. Should be a short patch for e.g. Mineclonia to leave the rest of stuff as-is but write the PNG in tandem and just use that. 20:27 erlehmann sfan5, btw, i have reports that minetest has much worse performance on reform2 than on my thinkpad 20:27 erlehmann Warr1024 if hecks was forced to follow a sensible deprecation policy that would probably have been discussed already 20:28 sfan5 in what way? rendering? if so no surprise, the graphics stuff that comes with SoC is bad (exceptions are rare) 20:28 erlehmann someone got like 7 or 8 fps out of the box 20:29 erlehmann i get 27 even with mineclonia on the T60, which is among the heaviest games 20:29 sfan5 it maybe also be a coincidence that the way MT goes about rendering works particularily bad there 20:29 erlehmann well it worked on reform1 20:29 MTDiscord Pretty sure we have been having the TGA vs PNG discussion already. It's just that instead of spiraling in toward a conclusion it's spiraling out into another sneakgate or something. 20:29 erlehmann i will investigate if anything broke it some time soon 20:30 sfan5 if whatever SoC the new one is using isn't using something entirely different for graphics the performance should be strictly better 20:30 erlehmann the new one is ofc using an imx8, while the old one is using an imx6 20:30 MTDiscord I'd rather just patch the mineclones defensively so that they can not have to give a shit about whether the engine supports TGA or not in the future. The sooner that happens, the better the chance that it will actually affect nobody, or at least the fewer who will actually be affected, regardless of what/when deprecations actually happen. 20:30 sfan5 at this point I'd question whether the graphics driver is even complete 20:31 erlehmann also the old one uses X11, the new one wayland 20:32 erlehmann look i am just trying to point out that hecktest is literally trying to make me unable to run minetest on any of the devices i own 20:32 erlehmann which kinda makes it personal 20:32 erlehmann unless afterwards it works on the reform2 20:32 Menchers hecktest? 20:33 MTDiscord Heh, I never heard of the reform before, so I went and checked it out, and they show one actually playing minetest :-) 20:34 erlehmann hecks is the person who is gutting irrlichtmt and plans to rewrite the renderer for reasons of performace on newer device, but will probably not do the work of making it still run on older devices or devices who have abysmal OGLES performance 20:34 appguru Really, the issue is MT doesn't abide by semver but claims it did. Features are being removed in minor releases, which is not acceptable. Minor releases may not break compatibility. I have no issue with the major version increasing every year, but please do it properly. 20:34 appguru s/did/does 20:34 erlehmann what appguru says 20:35 MTDiscord I'd be fine with doing major version bumps pretty regularly too. Not necessarily every release, but maybe every 2 releases (i.e. once per year) I could live with. 20:35 erlehmann it's funny though, many devs say to “just be honest about breakage” something like “but then we will we at version 25.x.y. in a year” 20:35 erlehmann well either they are honest about breaking stuff – or they stop breaking stuff 20:35 appguru MTver: majormajor.majorminor.minorpatch 20:36 erlehmann any idiot can make a new feature work. but it takes care to make it a) maintainable b) provide a good upgrade path. 20:36 MTDiscord I only support 1 past minor release for my game anyway, and even that isn't always "actively" tested. I do make exceptions to that policy if some platform gets stuck behind, like Android did for a while, but it really does hold the whole game back... 20:36 sfan5 a major version bump is essentially reserved for network incompatibility or I dunno, a half rewrite of the mod api or something 20:36 sfan5 proposing more major bumps is essentially saying "please do more, bigger and large scale incompatible changes" 20:37 appguru Please refer to https://semver.org/: "MAJOR version when you make incompatible API changes" 20:37 appguru "MINOR version when you add functionality in a backwards compatible manner" 20:37 appguru "PATCH version when you make backwards compatible bug fixes" 20:37 appguru There is no room for interpretation here. 20:37 sfan5 no? 20:37 sfan5 what is an "API"? 20:38 appguru The Lua API is obviously an API. 20:38 erlehmann appguru do not go down that rabbit hole. i have seen that discussion technique. sfan5 is probably not malicious, but you can not win when the other person traps you into “what is the meaning of this word”. 20:39 erlehmann especially if they have experience with that word (i.e. the API in this case) 20:39 appguru Well, the meaning of API is quite simple: Application Programmer Interface. 20:39 sfan5 so are the media formats part of the API or not? 20:39 sfan5 after all the API did not change 20:39 sfan5 does API even cover the client? all code runs on the server after all 20:40 sfan5 also, there are two API-incompatible versions of Minetest at any given point depending on whether you build with or without LuaJIT 20:40 sfan5 I don't think semver supports quantum states 20:40 sfan5 erlehmann: correct 20:40 appguru This conversation technique is generally referred to as "reasonable doubt" IIRC 20:40 appguru Yes, the Lua vs. LuaJIT thing is definitely an annoyance 20:41 appguru The server API did change: dynamic_media_add won't accept a TGA file anymore 20:41 erlehmann that's unfortunate 20:41 sfan5 don't worry, we could leave that in 20:41 sfan5 just the client won't accept it anymore ;) 20:41 erlehmann they are generated dynamically after all 20:41 appguru Well, TGA is being added back, so it's fine 20:42 appguru But media is definitely part of the interface. 20:43 sfan5 reverse question: since the network protocol is not an API could be break it in a minor version? 20:43 sfan5 as should be obvious by now my point is that throwing the semver definition at MT does not work 20:43 erlehmann told you you can't win 20:43 sfan5 maybe it would be worth to write down "Semver-inspired" rules that we actually use but I fear that would just result in more exceptions being made 20:43 erlehmann btw the technique is deeply unfair 20:44 erlehmann sfan5 what you are doing is moving the goalposts. you could definitely write down hard rules. 20:44 erlehmann but you would have to keep to them. 20:44 sfan5 yes 20:44 appguru "reverse question: since the network protocol is not an API could be break it in a minor version?" 20:44 appguru No we could not 20:45 appguru Because the API is documented by the effect it has on clients 20:45 erlehmann http://sentimentalversioning.org/ 20:45 erlehmann > You may explain the system you create, if the beauty is enhanced by understanding it. You may just improvise new numbers from your mood on that day. The only important thing, is that the version number must be meaningful to you, the author. 20:46 erlehmann i suggest to read the page to lighten the mood 20:46 erlehmann the examples are comedy gold 20:46 sfan5 somewhat related https://0ver.org/ 20:47 MTDiscord I actually use monotonic myself, but I could have some fun with senver 20:47 appguru "Jeremy calls his versioning system romantic (which is very sentimental of him) It looks like semver (major.minor.patch), but with a personal meaning. For him, a patch is a change you probably won’t notice, and a minor change is when you change it a little bit. A major change is when you make a major release (notice the circular definition!)." 20:47 appguru Now that reminds me much of Minetest ;) 20:47 MTDiscord I've seen projects that use senver actually kind of explicitly before (without necessarily knowing of that page). You get version numbers with emoji in them and stuff. 20:48 MTDiscord Look, it's simple. A minor change is any change that's not old enough to drink yet. A major change is a change that's bigger than a captain change but smaller than a lieutenant colonel change. 20:49 appguru Warr1024: If I get monotonic right, it just fits minor + patch in a single number? 20:49 MTDiscord er, wait, it's not actually monotonic, it's calver, I get those mixed up. 20:49 MTDiscord timestamp + commit hash 20:50 MTDiscord it always monotonically increases (the versions are lexically sortable except in extreme cases) but it doesn't say anything about distance. 20:50 MTDiscord In theory I could add a second versioning scheme post-hoc, but it seems complicated to have a project with 2 versioning systems. 20:51 appguru Just mush them all together: 0-YYYY-MM-dd-major.minor.patch-42-69 20:56 erlehmann i have updated my pixelflood mod to crash minetest again 20:56 erlehmann this will be a fun game 20:56 MTDiscord erlehmann, I got UBSan chugging away, and I'm working on the first error, namely the integer overflow in noise2d. 20:57 erlehmann josiah_wi yay 20:57 MTDiscord It was so subtle I had to go double check the assembly to convince myself of the issue lol. Almost scary. 20:57 erlehmann josiah_wi tell more? 20:57 MTDiscord I am asking myself how many times I've caused that exact same behavior. 20:57 erlehmann haha yeah 20:57 MTDiscord Overflow during an intermediate arithmetic operation. 20:58 MTDiscord The result is stored as unsigned, but all intermediate computations are signed. 20:58 erlehmann solution, ubsan in production, there is some “fast mode” where it just crashes i think and has little runtime overhead (but do not believe me, test that!) 20:58 MTDiscord I'm using the mode where it crashes on every error it encounters. 20:59 erlehmann that's a good way to work yourself through the binary lol 21:00 MTDiscord Anyway, I want advice from a core dev, because fixing this will very likely be a behavior change in the perlin noise. 21:01 MTDiscord I don't know the perlin noise math, and I have no clue what kind of change this is going to be, but I'm pretty sure the results of the arithmetic will be different after it's fixed. 21:03 sfan5 that doesn't sound acceptable 21:03 MTDiscord erlehmann, will UBSan make any kind of deductions about possible values, or will it only error when it observes UB actually happening? 21:04 MTDiscord I fixed it temporarily by casting one operand to unsigned int, but to properly fix it I'm assuming I would have to reason about all possible values of all operands. 21:05 MTDiscord sfan5, the argument for fixing it would be that right now, the perlin noise equation isn't even strictly defined. XD 21:05 erlehmann josiah_wi ubsan comes to you at runtime to tell you that your code did not actually compile. 21:06 sfan5 that is a good argument for fixing it, not for breaking the values 21:06 erlehmann josiah_wi pleases ask the ubsan docs about what it actually does behind the scene, not me 21:06 sfan5 maps are supposed to stay consistent, I don't know how strictly this promise is kept but it's yet another unwritten ruke 21:06 sfan5 rule* 21:06 erlehmann sfan5 in conclusion, a regression test would enable fearless refactorign 21:07 erlehmann maps already have different layout depending from where you approach a mapblock lol 21:07 erlehmann it's very subtle 21:07 erlehmann but prob irrelevant if no one noticed, it is hard to reproduce anyway 21:08 MTDiscord So, skip over this issue for the time being? 21:08 erlehmann why? 21:09 erlehmann there are only a limited amount of inputs and outputs, so you can actually test them all in reasonable time 21:09 MTDiscord Because the core devs won't support fixing it? 21:09 erlehmann i agree with sfan5 that maps should stay consistent 21:10 erlehmann so the fix should probably approach the most common miscompilation of the function 21:10 erlehmann or whatever you wanna call it 21:10 sfan5 if this is about blockseed there are 1208925819614629174706176 distinct inputs 21:10 erlehmann but i a defined, stable, never to be broken way 21:10 sfan5 we'll wait for you to test them all 21:11 MTDiscord sfan5, do you know off the top of your head whether the x and y arguments have set limits by the application or whether any integer value is possible? 21:11 sfan5 [-4095, 4096] or whatever 21:11 sfan5 so in fact are lied, that number is too big 21:11 sfan5 I* 21:12 MTDiscord Hang on a sec. How do we know maps are consistent? Because this is undefined behavior, different compilers could produce different maps for the same seed. (?) 21:12 MTDiscord 3d noise has the same UB btw, I can see it here looking at the code. 21:13 sfan5 ¯\_(ツ)_/¯ 21:15 MTDiscord Does the seed need to be signed? It's explicitly signed here, but if I could cast it to unsigned that would completely fix it. 21:16 ShadowNinja Signed integer overflow usually either 1. works as you'd expect or 2. triggers dead code elimination and causes the compiler to delete a chunk of code that was actually reachable. 21:16 ShadowNinja We can probably cast to/from unsigned with a reinterpret_cast and just assume 2's compliment representation. 21:18 erlehmann ShadowNinja it basically never works as you'd expect, because you can have BOTH: see my “if (1 + INT_MAX < 1)” example from earlier 21:18 erlehmann ShadowNinja the cast thing would have to be verified to actually behave like the old result though 21:18 erlehmann to not break mapss 21:19 sfan5 "it's possible therefore it's always the case" 21:19 erlehmann true 21:19 erlehmann demons will fly out eventually out of your nose, given enough compilations 21:19 MTDiscord Can I assume that the seed will always be positive? 21:20 erlehmann jokes aside, the result of what compilers do with signed integer overflow stuff used to change in minor versions of compilers bc they could literally do what they want, maybe that changed at some point? 21:20 MTDiscord Actually nevermind. I'm going to cast to long. 21:20 MTDiscord I better explicitly make it s64. 21:21 erlehmann josiah_wi please have a regression test or at least confirm that the assembly is the same, sfan5 map breakage fears are well-founded and i share them! 21:21 ShadowNinja Yep, long may be the same size as int depending on platform. 21:21 sfan5 I'm pretty sure if you cast it right you can get the same assembly as before 21:21 erlehmann better explicitly use types of a width 21:21 sfan5 an that would be definitely safe 21:22 erlehmann otherwise you may get a windows/linux problem 21:22 MTDiscord So best bet is to just make the overflow explicit so ubsan doesn't complain about it? 21:22 MTDiscord It's simply impossible to not overflow and have the same behavior. 21:23 MTDiscord Now practically, it might turn out there's no noticeable change at all. I can test that. 21:23 sfan5 changing it into a defined overflow is the way to go, if that works 21:25 erlehmann if there is no change in behavior, you basically lock it in, which is good 21:25 erlehmann bc then future compilers will not say that this is not the way 21:25 erlehmann sfan5, your move https://github.com/minetest/minetest/issues/11628 21:26 erlehmann and by “your move” i mean, i accidentally filed it on the wrong repo 21:26 erlehmann again 21:26 erlehmann sorry 21:26 erlehmann or did i? 21:26 erlehmann sfan5, i actually think this should not even reach irrlichtmt 21:27 erlehmann so maybe it is the right repo 21:27 pgimeno @josiah_wi can you point out to the line causing the trouble? 21:28 erlehmann yeah i think pgimeno fixed it already or not? 21:28 erlehmann question was if the fix was the same 21:28 MTDiscord noise.cpp line 167 and line 177. 21:28 MTDiscord Sorry, 168 and 178, because the lines wrap. 21:29 sfan5 well I won't be adding a size limit in Minetest 21:30 pgimeno oh fsck, these are ints 21:31 MTDiscord Yeah. I don't know how to make the overflow explicit. If I cast before multiplying I change the operands which is no good, but if I cast after it's too late? 21:32 MTDiscord Casting the seed to s64 fixes the UB but it also changes the assembly (and the behavior, ofc, because now it doesn't overflow). 21:32 pgimeno I'd just change s32 to u32 and call it a day 21:33 MTDiscord That's why I've been asking whether a negative seed is possible. Because if so, that solution is also going to break map consistency (although probably not in practice). 21:33 pgimeno I don't say it lightly even if it might seem so, the operation is with a s32 and the product is going to have 32 bits 21:34 pgimeno it doesn't matter if the seed can be negative 21:34 erlehmann josiah_wi you could make at least a unit test that checks out the boundaries 21:34 erlehmann i.e. test for cases that overflow and *just so* do not overflow 21:34 pgimeno changing the API is probably not wise, so better to cast the seed to u32 instead 21:35 MTDiscord Ok, thanks! 21:37 MTDiscord pgimeno, irrMath.h:329, have you fixed this or shall I start on it? 21:37 MTDiscord (another signed integer overflow) 21:37 pgimeno @josiah_wi I have only worked on the first one erlehmann reported 21:39 pgimeno for noise.cpp it's possible to just change line 38 to define NOISE_MAGIC_SEED as 1013U 21:39 erlehmann they are basically the same i think 21:39 erlehmann or not? 21:39 erlehmann ah 21:39 pgimeno yes 21:39 erlehmann why is that code duplicated anyway 21:40 MTDiscord I feel like facepalming. The function is computing the maximum value of an s32 (and the one for computing the minimum has the same UB). 21:41 MTDiscord Oh wait no it's not. It's taking the max of two s32 values. 21:41 pgimeno to be clear, this is the patch I propose: http://www.formauri.es/personal/pgimeno/pastes/fix-two-overflows.diff 21:41 MTDiscord My bad. Anyway, I'm pretty sure there's something for computing a max in the standard library.... 21:42 ShadowNinja https://gcc.godbolt.org/z/s4jcaWzhM 21:43 ShadowNinja clang compilation is identical. gcc compilation is slightly different, but it looks like the instructions are just reordered. 21:43 MTDiscord My one concern with the proposed patch is that it's kind of an obscure way to fix the issue and later nobody is going to understand why it was done, but I will do it that way. 21:45 pgimeno @josiah_wi it can be explained in the commit text 21:45 pgimeno or even in a comment 21:46 pgimeno Minetest is sorely lacking in the comment area 21:46 pgimeno // The U causes promotion to unsigned of the operation, avoiding UB 21:47 pgimeno (example comment) 21:47 MTDiscord Let's face it. The function in mapgen.cpp should be using the defined value instead of a magic number. 21:47 MTDiscord I have to put the comment in 2 places because of this. 21:47 pgimeno right, but the defines aren't in any header file 21:47 MTDiscord Yeah. 21:48 pgimeno and you know how erlehmann complains when a header file is changed :) 21:51 erlehmann pgimeno i complain anytime, look i trust all of you to work this out 21:51 MTDiscord Irrlicht has a whole bunch of functions for taking the max/min of two values. Is it OK to start replacing these with STL functions, or are they important for performance or something? 21:51 erlehmann i mean with this issue everyone involved understands the rules of basketball and cares about backwards compat 22:10 erlehmann did you know that a 165 byte BMP can have apparent image dimensions of 1111498827×1083981? neither did minetest! 22:10 erlehmann https://github.com/minetest/minetest/issues/11629 22:45 pgimeno feh swallows that one without problems, it doesn't even take long to open, about 4s 22:48 MTDiscord networkpacket.cpp line 66: null pointer passed as argument 1, which is declared to never be null 22:49 erlehmann ha, this proves you actually did run it 22:49 erlehmann i did not mention that one on purpose so far 22:49 MTDiscord Ofc I ran it. ^^ Why didn't you mention that one? 22:51 erlehmann well it is something weird with network packets 22:51 erlehmann so naturally, i want this for the client-sent server-side mods! 22:52 erlehmann also it is the easiest fiend 22:52 erlehmann find 22:52 erlehmann happens every time 22:52 erlehmann so congratulation, you are the only one who actually tried ubsan apparently 22:54 MTDiscord It's always possible someone else found it too and kept silent... 22:59 MTDiscord How do you get the tracebacks? 23:02 erlehmann josiah_wi start the instrumented binary with UBSAN_OPTIONS='print_stacktrace=1' 23:27 pgimeno here's the backtrace of #11628: http://www.formauri.es/personal/pgimeno/pastes/backtrace-image-crash.txt 23:27 ShadowBot https://github.com/minetest/minetest/issues/11628 -- Pixelflood 2 – Segfault Boogaloo: Sending lottapixel.jpg from server segfaults client upon connection 23:28 pgimeno in fact I haven't tried if pixelflood 1.0 crashes too 23:30 pgimeno it's a bit weird that it crashes because it reports an error and a warning before crashing 23:33 pgimeno yeah, pixelflood 1.0 does not crash, but why does 1.0 not crash and 2.0 does? after the error it should have created a dummy texture just like with 1.0, but it seems that hasn't happened 23:33 pgimeno I wonder if this still holds: https://github.com/minetest/irrlicht/blob/master/source/Irrlicht/CImageLoaderJPG.cpp#L94 23:45 erlehmann pgimeno pixelflood 1.0 was fixed by sfan5 already 23:45 erlehmann by limiting the image dimensions to 32000×32000 23:45 pgimeno yeah my point is that the crash does not happen while loading 23:45 erlehmann ah hmm 23:45 pgimeno it actually attempts to use the image 23:45 erlehmann and then crashes? 23:46 pgimeno yes, I guess it does because it's incomplete 23:47 pgimeno what it should do in case of error is what happens after sfan5's fix: it should replace it with a dummy texture 23:48 erlehmann i need to make a prequel btw 23:48 erlehmann pixelflood 0: divided 23:49 erlehmann just need to find an image format that somehow divides by something 23:49 erlehmann and i can also make “2 pixel 4 flood – old filename, new segfault” i guess 23:50 pgimeno you're giving good arguments for taking out TGA :) 23:51 pgimeno the fewer graphic formats, the lesser code to maintain 23:52 erlehmann pgimeno actually TGA is probably the one where i would demonstrate how to fix this issue 23:52 erlehmann it is the one for which you can feasibly write a validator 23:53 erlehmann to fix these kinds of issues once and forever, one basically needs to write a function that takes the input file and returns either true or false 23:53 erlehmann and only determines if it is valid (i.e. expected) input 23:54 erlehmann since the TGA files created by MineClone2 and MineClone2 23:54 erlehmann 5 i mean 23:54 erlehmann are 18 bytes + payload 23:54 pgimeno if it's not feasible in other cases, why even bother? 23:54 erlehmann it is pretty easy 23:54 erlehmann well it is possible, just not in very little time 23:55 erlehmann to write it i mean 23:55 erlehmann the validation is always fast 23:55 erlehmann and you do not neet to match the full file format, only the part of the spec that minetest actually understands 23:56 erlehmann like, if minetest can't do palette images or whatever weird feature a file format might have, let the validator reject that 23:56 erlehmann before it reaches code where it might crahs 23:56 erlehmann basically, that way you get an application firewall 23:56 pgimeno you don't have to validate files if you have robust decoders, which I don't know if is the case 23:56 erlehmann that is plain wrong 23:56 erlehmann do you have python3 installed? 23:56 pgimeno yes 23:57 erlehmann from base64 import b64decode 23:57 erlehmann b64decode("V=m=0=") 23:57 erlehmann stuff is *horribly* broken 23:58 erlehmann becaue there is no way that “V=m=0=” is valid basse64 23:58 erlehmann yet it is decoded to “Vm” 23:59 erlehmann now imagine you have python – you must put a regex that recognizes valid base64 before the b64decode() invocation, otherwise bad things will happen. 23:59 erlehmann but once you do, you don't care about how your decoder reacts to malformed input 23:59 erlehmann after all, it handles all the valid innput correctly 23:59 MTDiscord take it from me - if someone is going to validly upload broken things to CDB it'll get caught by the review process as it probably contains crashes from corrupt or invalid files 23:59 erlehmann and the invalid one does not reach it 23:59 MTDiscord things like this in a small community that generally knows each other isn't going to go far 23:59 erlehmann is that a challenge