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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <Jordach> 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 <josiah_wi> 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 <josiah_wi> 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 <Warr1024> 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 <josiah_wi> 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 <josiah_wi> 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 <Warr1024> 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 <josiah_wi> 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 <josiah_wi> 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 <Warr1024> yeah, this just looks wrong
02:05 MTDiscord <Warr1024> because it's being passed an int, but it produces the same behavior as if it were passed an unsigned int
02:05 MTDiscord <Warr1024> 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 <Warr1024> I don't remember the nuances of C that much tho
02:06 MTDiscord <Warr1024> heh, I added a case for -5 and it generated a LUT instead.
02:07 MTDiscord <Warr1024> Now we've suddenly gone from integer overflow risk to out-of-bounds memory access risk
02:08 MTDiscord <josiah_wi> 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 <Warr1024> 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 <josiah_wi> @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 <Warr1024> 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 <climits>
02:37 erlehmann #include <cstdio>
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 <Jordach> for every hack made for speed
02:58 MTDiscord <Jordach> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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] <pgimeno> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> Is there a macro that's literally just an underscore?
14:09 sfan5 possibly
14:09 pgimeno most likely translation stuff
14:09 MTDiscord <josiah_wi> make_pair("name", _("some string"))
14:10 MTDiscord <josiah_wi> 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 <Jonathon> skyboxes use the largest images that i know about
14:36 MTDiscord <josiah_wi> I am missing something. The second insert into the std::map of options always causes the memory error
14:36 MTDiscord <josiah_wi> Doesn't matter which insert. It's always the second one.
14:37 MTDiscord <josiah_wi> Is it possible that std::map is responsible!?
14:41 pgimeno @josiah_wi which issue are you investigating?
14:42 MTDiscord <josiah_wi> The uninitialized value on startup.
14:42 pgimeno what issue # ?
14:44 MTDiscord <josiah_wi> 11624
14:45 MTDiscord <josiah_wi> Hah, doing all the options in a single insert avoids the error,
14:45 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> Nevermind, error came back.
14:57 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <erlehmann> 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 <josiah_wi> 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 <josiah_wi> The static lib either didn't fix it, or I messed some up.
19:48 MTDiscord <josiah_wi> 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 <Warr1024> Holy shit, this Targagate thread on GitHub ... what's all this talk about breaking old worlds?
20:17 MTDiscord <Warr1024> 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 <Warr1024> Just read the maps from TGA, then write them to TGA (for storage) and to PNG (for the clients to display)
20:17 MTDiscord <Warr1024> 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 <Warr1024> Server owners are probably just going to have to maintain their servers
20:20 MTDiscord <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> I can tell you from experience that being on Debian means you use FlatPak :-D
20:24 MTDiscord <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> 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 <Warr1024> er, wait, it's not actually monotonic, it's calver, I get those mixed up.
20:49 MTDiscord <Warr1024> timestamp + commit hash
20:50 MTDiscord <Warr1024> it always monotonically increases (the versions are lexically sortable except in extreme cases) but it doesn't say anything about distance.
20:50 MTDiscord <Warr1024> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> I am asking myself how many times I've caused that exact same behavior.
20:57 erlehmann haha yeah
20:57 MTDiscord <josiah_wi> Overflow during an intermediate arithmetic operation.
20:58 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 3d noise has the same UB btw, I can see it here looking at the code.
21:13 sfan5 ¯\_(ツ)_/¯
21:15 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> Actually nevermind. I'm going to cast to long.
21:20 MTDiscord <josiah_wi> 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 <josiah_wi> So best bet is to just make the overflow explicit so ubsan doesn't complain about it?
21:22 MTDiscord <josiah_wi> It's simply impossible to not overflow and have the same behavior.
21:23 MTDiscord <josiah_wi> 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 <josiah_wi> noise.cpp line 167 and line 177.
21:28 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> Ok, thanks!
21:37 MTDiscord <josiah_wi> pgimeno, irrMath.h:329, have you fixed this or shall I start on it?
21:37 MTDiscord <josiah_wi> (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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> Let's face it. The function in mapgen.cpp should be using the defined value instead of a magic number.
21:47 MTDiscord <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> 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 <josiah_wi> It's always possible someone else found it too and kept silent...
22:59 MTDiscord <josiah_wi> 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 <Jordach> 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 <Jordach> 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