Time Nick Message 00:11 paramat probably not 00:34 p_gimeno hm, the operator<< does not accept packets larger than 65535 bytes 00:52 p_gimeno are you working on a fix for #8325 sfan5? 00:56 p_gimeno just confirmed 0a5e771 as the cause 00:58 paramat good, thanks 01:01 p_gimeno so I'm not very familiar with the packet code, but it seems like fixing this will break the network protocol again 01:28 sofar I'll hve time later to look 01:28 sofar not right now 01:47 paramat ok thanks, no rush 02:28 paramat core devs: probably a good idea to stop merging PRs until the big bug is fixed and 5.0.1 released 02:31 paramat nerzhul and all, the android 5.0.0 apk on the releases page apparently has a serious bug, see https://github.com/minetest/minetest/issues/8326 02:32 paramat somehow we missed this https://github.com/minetest/minetest/issues/7965#issuecomment-455841251 02:36 paramat hopefully it only affects a minority of android users 02:41 paramat i guess the beta testing will show 03:14 paramat considering http://irc.minetest.net/minetest-dev/2019-03-05#i_5505552 shall we merge https://github.com/minetest/minetest/pull/8258 "Confirm registration: Remove server and name strings to fix Windows bug" for 5.0.1? i think that should have been merged for 5.0.0 03:17 p_gimeno you can replace them by %s instead too 03:18 p_gimeno the positional parameters just help translators to change the order if necessary, that's their whole point 03:20 p_gimeno isn't there some other formatting mechanism in the C++ side for variable stuff in translatable strings? 03:21 p_gimeno like some function that accepts @1 and @2 03:50 sofar man gettext 03:59 sofar paramat: we can make a 5.0.1 branch for fixes 04:00 sofar easy enough, isolate the few fixes we need/want, apply to master & test, then apply to 5.0.1 branch or stable-5 and release 04:00 sofar don't we already have a stable-5 branch? 04:22 sofar maybe not in minetest_game then 05:09 paramat ok maybe 05:20 sofar we do have it already 05:20 sofar which is great, I've rebased those 2 PRs to more comprehensible versions 05:20 sofar and they look OK, but someone needs to verify they fix the issue at hand 05:23 sofar ok, game#2327 and game#2328 replace them 05:23 sofar they look fine from my view, especially after my few simplifications 05:25 sofar nerzhul: I put some errorstream<< debugging stuff in the emerge thread, and that killed it easily 05:25 sofar I wonder if a lua callback that does minetest.log() would similarly trigger that 05:34 paramat thanks 05:37 sofar if bell07 can retest, we can merge onto stable-5 just fine, I think 05:37 sofar which reminds me, don't use the web interface to merge it (that'll merge it to master, which is needed too ofc) 05:41 paramat ok 06:27 nerzhul just push fixes on master and stable-5 06:31 nerzhul yes the strings serialization doesn't accept more than u16 06:32 nerzhul if needed we can break and increase that to u32 or just reuse the deserializestring function, i tend to prefer the first solution 06:33 nerzhul anoying we didn't detected that in RC :( 06:37 sofar I don't see any shocking 5.0.0 reports on breaking mods 06:37 sofar maybe they'll come later 07:00 nerzhul the protocol breakage must be fixed today 07:00 nerzhul i will fix it this morning. Can somebody test the PR this morning ? 07:01 nerzhul (with the problematic mod) 07:26 sofar what PR? 08:40 nerzhul the pr i will produce 08:40 nerzhul if you can reproduce the problem and i can produce the breakage fix 08:49 nerzhul we have a putLongString function in network packet for such behaviour 08:49 nerzhul i suggest to unify both string serialization operators to prevent problems in the future 09:01 nerzhul sofar i will provide PR asap 09:01 nerzhul it's a refactor 09:01 rubenwardy does this break network compatibility again? 09:03 nerzhul unfortunately in both possible fix there is a breakage 09:04 nerzhul one is mitigated by the 65536 string limitation, other permits strings 64MB length in every case and cleanup the code 09:04 nerzhul i will push the second option to fix this definitively 09:04 nerzhul this will prevent future coding errors 09:05 nerzhul because the problematic here is that somebody changed the serializaiton of the string in one packet and the underlying code has 65536 limitaiton 09:05 rubenwardy if there's a breakage, this would need to be 6.0 09:05 nerzhul we have two choice 09:06 rubenwardy is it possible to just use protocol numbers? 09:06 nerzhul either our mods are broken in detached inventories 09:06 nerzhul and this needs a protocol breakage in every case 09:06 nerzhul or we fix definitively the issue 09:06 nerzhul release was done yesterday and there are not many users in this version 09:07 nerzhul we can do a urgent fix today 09:07 nerzhul and in both case we should notify users that the 5.0.0 has a very anoying problem 09:07 rubenwardy well, sooner the better 09:08 rubenwardy is the packet broadcasted? 09:08 nerzhul if i remember it is for detached inventories 09:08 nerzhul running unittests now 09:17 rubenwardy putRawString doesn't put a length on first, but << does. So as a hacky way to support both, you could put an unused integer before putRawString 09:17 rubenwardy only issue is what to do when the length would break on 5.0.0 09:17 nerzhul i prefer to unify all strings serizliation but it's a more heavy work 09:18 rubenwardy yeah 09:18 nerzhul hopefully we have nice unittests to verify all of this 09:27 rubenwardy only issue is what to do when the length would break on 5.0.0 09:27 rubenwardy given that this happens with the creative inventory... 09:29 rubenwardy well, we could just let the client crash and tell people to update to 5.0.1 to "fix the crash" :) 09:29 nerzhul ugly 09:30 rubenwardy better than essentially having 6.0.0 now 09:31 rubenwardy it won't work actually, as putRawString is probably null terminated anyway 09:32 nerzhul putRawString allow non null terminated if i remember 09:33 rubenwardy well, my proposal would be to redundantly encode the string like this: (u16)len STR \0 09:33 rubenwardy the (u16)len STR would allow it to be read on 5.0.0 09:33 rubenwardy then the len would be ignored and STR \0 used on 5.0.1 09:33 rubenwardy the issue is skipping the \0 09:33 nerzhul i'm working on u32 len everywhere 09:33 rubenwardy and it's basically a hack for idealogical reasons 09:33 nerzhul to remove this issue definitively 09:34 rubenwardy oh interesting 09:34 rubenwardy so it's a break to avoid this forever 09:34 nerzhul i refactored the network packet but it has effects on other code parts 09:34 nerzhul the refactor on the serialization should be done later in 5.1, not important now 09:34 nerzhul i just want to ensure tests pass, they cover the serialization code correctly :D 09:41 nerzhul hmm it's not ready yet 09:41 nerzhul #8330 is out 09:41 nerzhul but there are some problems currently 09:48 nerzhul where is the serialization problem in packet... hmmm 09:51 nerzhul wtf the size is totally wrong 09:53 nerzhul roh 09:53 nerzhul omg we have a badly serialized sendReady 09:53 nerzhul it doesn't used the proper networkpacket serialization methods 09:53 nerzhul xD 09:53 nerzhul interesting to find a such case by a side effect 09:54 nerzhul ok it works better 09:55 nerzhul GH permits to have draft PR now :o 09:56 nerzhul sofar please test https://github.com/minetest/minetest/pull/8330 09:56 nerzhul or someone else who is able to reproduce the detached inventory problem 10:03 rubenwardy I'm not sure whether removing the distinction between short and long strings is a good idea 11:44 sfan5 not having read the backlog: 11:44 sfan5 how did this work before in 0.4.17.1? 11:45 p_gimeno it used putRawString, https://github.com/minetest/minetest/commit/0a5e77132ae8c495c50cfc58bbe4ce1bfcd377e3 11:45 rubenwardy from a comment, it looks like putRawString is STR\0 11:45 rubenwardy and << is u16 STR 11:45 rubenwardy but I may be wrong 11:46 sfan5 I see 12:37 nerzhul you are right, it's the original behaviour 12:38 rubenwardy what's the difference between putLongString and putRawString? 12:39 rubenwardy hmm 12:40 rubenwardy putLongString is os << string.size(); putRawString(os, string); 12:40 rubenwardy where string.size() is u32 12:40 rubenwardy so putLongString is u32 STR 12:41 sfan5 i'd expect putRawString to be just the string, no terminator, no length 12:42 rubenwardy yeah 12:42 rubenwardy that is how it is 12:42 rubenwardy the \0 would only be if the string contains \0 12:42 p_gimeno IIUC it's possible to fix it without breaking the protocol again 12:42 rubenwardy it definitely is 12:42 rubenwardy 5.0.0 would still crash on too large inventories 12:42 rubenwardy but it's not like we can retroactively fix old bugs anyway 12:43 rubenwardy I think it would be a good idea to remove the << operator for strings, maybe 12:43 rubenwardy because there's multiple encoding forms 12:43 p_gimeno sounds good, that's a troublemaker 12:45 p_gimeno what's the maximum UDP packet that can be reliably sent? 12:45 rubenwardy 512 12:45 rubenwardy but fragmentation exists 12:45 nerzhul rubenwardy: it's why in my pr i removed the long and pushed one wait to serialize string not 2 12:45 nerzhul one way* 12:45 nerzhul just test it and merge 12:46 nerzhul 5.0.0 has just be released we have time for a such fix if we test/merge/release quickly 12:53 sfan5 I oppose breaking network compatibility again 12:53 sfan5 also the put string functions should probably raise a warning when the given string is too long 12:53 rubenwardy I agree with sfan5 12:55 nerzhul then just revert the serialization mode of this function. this is a breakage, again 12:55 nerzhul both are breakage, one is more important than the other 12:55 sfan5 no it's not breakage 12:55 sfan5 you add a check for the protocol version 12:55 nerzhul dont add compat code for a 1 day problem 12:56 nerzhul guys we released yesterday, not 3 weeks agos 12:56 nerzhul ago* 12:56 nerzhul and i can add the protocol bump too :) 12:56 nerzhul i fogot it 12:57 sfan5 yet another network breakage will not make the mess that is 5.0 any better 12:57 sfan5 besides, do you want to call the new version 6.0? or just ignore the versioning scheme? 12:57 nerzhul as i said i can add protocol version bump :p 12:57 sfan5 support for 5.0 starts the day it is released, it sucks that this error wasn't caught before 12:58 sfan5 but it's fairly easy to fix this in a backwards "compatible" way 12:58 nerzhul then go 12:58 nerzhul keep my pr for next 2 years 12:59 sfan5 such a cleanup of networking would've been perfect for 5.0 but it's not something we can do now, that's obvious 12:59 nerzhul yep 13:00 nerzhul then just fix that function and merge + release 13:00 nerzhul tell me when build for android, and please bump the version code 13:11 rubenwardy this is what I meant, btw: https://github.com/minetest/minetest/commit/e38f1b1e48ae8980c6cf2cbd339db21578e72719 13:12 nerzhul very ugly and insecure 13:12 rubenwardy it is a hack though, so a protocol version method would be better 13:12 rubenwardy nerzhul: sounds like me 13:12 nerzhul protocol bump with reverting to previous behaviour is less hacky 13:12 rubenwardy ugly and insecure 13:13 rubenwardy the copying here is kinda disgusting 13:22 sfan5 the server code is not actually laid out to send different packets to different clients per protocol version 13:22 sfan5 m_clients.sendToAll should not exist 13:23 rubenwardy yeah 13:23 sfan5 so rubenwardy's fix might actually be preferabler 13:23 rubenwardy I haven't tested other than --run-unittests 13:23 rubenwardy actually just cackled at "preferabler" 14:09 nerzhul i prefer protocol bump than ugly hack difficult to understand in 2 years :p 14:10 rubenwardy the ugly hack is just adding a u16 which is ignored 14:10 rubenwardy note that << is literally the same as what I implement there 14:11 rubenwardy just with putRawString(str, str.size()) instead of putRawString(str); 14:13 rubenwardy the benefit is that we don't end up with 3 different incompatible client versions 14:14 rubenwardy this bug doesn't even effect my server, but the incompatible client versions would 14:42 rubenwardy merging trivial bug fix in 10: https://github.com/rubenwardy/minetest/commit/c735497a65c8133c0df6b7ee7fc87dc4725994e6 15:57 rubenwardy nore, sofar: it would be good to hear your input on #8331 vs #8330 15:58 rubenwardy https://github.com/minetest/minetest/pull/8331 15:58 rubenwardy https://github.com/minetest/minetest/pull/8330 16:05 rubenwardy sfan5: made your changes, now testing 16:17 nore hmmm 16:17 nore I'm not sure 16:18 nore moving to 32 bits to store string length seems like the best option to me (it is more future-proof) 16:18 rubenwardy but it also breaks network compatibility, so you end up with 3 different versions 16:18 nore however, it also has a few problems, namely, it completely breaks network 16:18 nore exactly 16:19 rubenwardy and it also shouldn't be released as a 5.0.1, but 6.0.0 16:19 rubenwardy Minetest has survived this long with the 2 different string types 16:19 nore I think I would have been for it, had it been before the 5.0.0 release 16:19 nore now... I'd say wait for next important backwards-incompatible protocol bump and merge it with it 16:19 nore and use your fix in the meantime 16:29 sfan5 rubenwardy: while unittests would be nice I don't think there are any you can write here 16:30 rubenwardy yeah, I can't think of any 17:01 rubenwardy https://github.com/minetest/minetest/pull/8333 17:01 rubenwardy not sure if that counts as trivial 17:01 rubenwardy probably not 17:08 nore rubenwardy: simple enough for me 17:08 rubenwardy I don't think that should be in 5.0.1 17:08 rubenwardy idk actually 17:09 nore well it's a bugfix 17:09 sfan5 it's additionally "crash" potential since previously strings would silently get truncated 17:09 sfan5 maybe make it raise a (loud) warning instead? 17:09 nore nah, I'd say crash is better 17:09 nore it will make any bugs with it that remain more visible, so that they can be fixed for 5.0.2 17:10 rubenwardy I can see this being triggered by large book meta 17:10 sfan5 right, that makes sense 17:12 nore hmmm, yes, I do want a crash 17:13 nore there might be exploit potential if a string is followed by other data in a packet, and the length is truncated 17:13 nore because then it is possible to inject whatever you want in the other data and that can become quite dangerous 17:13 nore so a crash is much safer, imho 17:15 sfan5 if the length is truncated, the string is truncated too, actually 17:18 nore ah ues, indeed 17:18 nore *yes 17:18 nerzhul a simple protocol bump with minimum version is needed for my fix 17:18 nore but there could be buffer overruns in deserialization code of those strings 17:19 rubenwardy *this << msgsize; putRawString(src.c_str(), msgsize); 17:19 rubenwardy it would be as if the string was just shorter 17:19 rubenwardy or truncated 17:21 nerzhul and note: for this network fix, asap we fix it asap we prevent player dispatch on 5.0.0 & 5.0.1 17:27 rubenwardy except, if you break the network it would be 6.0.0 17:48 nerzhul it doesn't break network 17:48 nerzhul we have protocol bump here 17:48 nerzhul in 5.0.0 we did a major break, here there is no problem 17:49 nerzhul it's like everytime we can, protocol bump + raise min version 17:49 nerzhul we did it in 0.4.15 if i remember for example :) 17:55 sfan5 I don't see any compatibility code to allow 5.0.0 clients to connect in your PR 17:57 sfan5 what happened in 0.4.16 was this: https://github.com/minetest/minetest/commit/f8ad01ab7c4cf012781bd4caa821544e676c9267 17:57 nerzhul i just missed the proto bump 17:58 sfan5 we bumped the minimum version to a 3 years old version after it had falled out of use 17:59 nerzhul i don't see the point here. Let 5.0.0 together and 5.0.1+ work 17:59 nerzhul for me nobody should use 5.0.0 with some heavy mods it's unusable 17:59 sfan5 yes, #8330 does not allow for this 18:37 p_gimeno is it OK if I provide GH issue links for issues and PRs for now? 18:37 p_gimeno https://github.com/minetest/minetest/pull/8330 -- Fix definitively string serialization coding problems by nerzhul 18:37 p_gimeno #8330 18:39 p_gimeno (with a HexChat Lua script I've written) 18:42 p_gimeno well if no one complains I'll do it until ShadowBot is back 20:46 nerzhul lol:) 20:48 rubenwardy #9999 20:48 p_gimeno https://github.com/minetest/minetest/issues/9999 -- Page not found · GitHub 20:48 rubenwardy #;xdg-open example.com 20:49 rubenwardy (this is where he times out) 20:50 p_gimeno heh no :P 20:52 VanessaE p_gimeno: if you can reach ShadowNinja out-of-IRC somehow, maybe you can convince him to fix ShadowBot 20:52 rubenwardy it would be better to add it as a module to MinetestBot 20:52 rubenwardy and only have one bot 20:52 rubenwardy maybe I should do that 20:52 VanessaE all the data's there, byte-for-byte, I just can't get the G*d damned thing to run properly. 20:52 rubenwardy !help 20:52 p_gimeno VanessaE: mind if I IM you privately? 20:52 VanessaE p_gimeno: shoot. 20:52 rubenwardy lol! 20:53 rubenwardy MTB isn't here either 20:56 sfan5 it never was 20:58 GreenDimond MinetestBot is superior 20:58 GreenDimond MinetestBot will rule over all 21:32 p_gimeno ooh 21:33 VanessaE #1234 21:33 p_gimeno https://github.com/minetest/minetest/issues/1234 -- Client side caching of mapblocks 21:33 VanessaE come on damn it.. 21:33 GreenDimond ~". You may want to quote your arguments with double quotes in order to prevent extra brackets from being evaluated as nested commands. 21:34 ShadowBot https://github.com/minetest/minetest/issues/1234 -- Client side caching of mapblocks 21:34 VanessaE \o/ 21:34 GreenDimond rofl 21:34 VanessaE it wasa just being slow to respond 21:34 VanessaE probably because the server's kinda bogged down right now 21:34 VanessaE so..yay, ShadowBot is back :D 21:35 p_gimeno woot 22:09 rubenwardy merging #8333 in 15 22:09 ShadowBot https://github.com/minetest/minetest/issues/8333 -- Fix incorrect string length check after cast by rubenwardy 22:09 rubenwardy I really need like a postit note for the correct times to wait 22:10 rubenwardy inspirations for a better commit message welcome 22:13 rubenwardy can't actually see any rules about waiting for something that's approved 22:13 rubenwardy only for trivial bug fixes