| Time |
Nick |
Message |
| 00:02 |
|
Alias joined #minetest-dev |
| 00:16 |
|
calcul0n_ joined #minetest-dev |
| 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:07 |
|
v-rob joined #minetest-dev |
| 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:22 |
|
Extex joined #minetest-dev |
| 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 |
| 03:15 |
|
tekakutli joined #minetest-dev |
| 03:58 |
|
Extex joined #minetest-dev |
| 04:00 |
|
MTDiscord joined #minetest-dev |
| 04:12 |
|
v-rob joined #minetest-dev |
| 04:28 |
|
erlehmann joined #minetest-dev |
| 05:17 |
|
longerstaff13 joined #minetest-dev |
| 06:19 |
|
v-rob joined #minetest-dev |
| 06:58 |
|
queria joined #minetest-dev |
| 07:14 |
|
specing_ joined #minetest-dev |
| 07:16 |
|
v-rob joined #minetest-dev |
| 07:22 |
|
sfan5 joined #minetest-dev |
| 08:06 |
|
hendursa1 joined #minetest-dev |
| 09:14 |
|
olliy joined #minetest-dev |
| 09:31 |
|
Kimapr[i] joined #minetest-dev |
| 09:55 |
erlehmann |
josiah_wi maybe try to reproduce this and see if you get similar results https://github.com/minetest/minetest/issues/11624 |
| 10:00 |
|
Kimapr[i] joined #minetest-dev |
| 10:20 |
|
calcul0n_ joined #minetest-dev |
| 10:59 |
|
adfeno joined #minetest-dev |
| 11:42 |
|
tekakutli joined #minetest-dev |
| 12:07 |
pgimeno |
erlehmann: what noise patch? |
| 12:08 |
|
calcul0n__ joined #minetest-dev |
| 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 |
|
adfeno joined #minetest-dev |
| 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 |
| 12:56 |
|
adfeno joined #minetest-dev |
| 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:10 |
|
proller joined #minetest-dev |
| 14:20 |
|
calcul0n joined #minetest-dev |
| 14:21 |
pgimeno |
@josiah_wi see https://www.gnu.org/software/gettext/manual/html_node/Overview.html second box |
| 14:22 |
|
Fixer joined #minetest-dev |
| 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:46 |
|
v-rob joined #minetest-dev |
| 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:54 |
|
proller joined #minetest-dev |
| 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:15 |
|
MTDiscord1 joined #minetest-dev |
| 15:36 |
|
hendursaga joined #minetest-dev |
| 15:50 |
|
appguru joined #minetest-dev |
| 15:51 |
appguru |
We don't use Travis CI anymore, right? |
| 15:54 |
sfan5 |
no and even if we wouldn't be affected |
| 15:54 |
|
flux__ joined #minetest-dev |
| 15:55 |
sfan5 |
the reason you're asking is https://twitter.com/peter_szilagyi/status/1437646118700175360 right? |
| 15:55 |
|
Extex joined #minetest-dev |
| 15:58 |
nrz |
yeah, didn't linked it because we moved long time ago because travis was... s**t ? |
| 16:57 |
|
v-rob joined #minetest-dev |
| 17:00 |
|
Extex joined #minetest-dev |
| 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. |
| 17:12 |
|
tekakutli left #minetest-dev |
| 17:16 |
|
tekakutli joined #minetest-dev |
| 17:20 |
|
calcul0n_ joined #minetest-dev |
| 18:18 |
|
v-rob joined #minetest-dev |
| 18:20 |
|
erlehmann joined #minetest-dev |
| 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:35 |
|
longerstaff13 joined #minetest-dev |
| 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:50 |
|
v-rob joined #minetest-dev |
| 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:15 |
|
specing_ joined #minetest-dev |
| 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. |
| 19:57 |
|
v-rob joined #minetest-dev |
| 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:14 |
|
v-rob joined #minetest-dev |
| 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:50 |
|
Extex joined #minetest-dev |
| 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:07 |
|
v-rob joined #minetest-dev |
| 23:15 |
|
hendursaga joined #minetest-dev |
| 23:26 |
|
AliasAlreadyTake joined #minetest-dev |
| 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:41 |
|
adfeno_ joined #minetest-dev |
| 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 |
