Minetest logo

IRC log for #minetest-dev, 2019-03-06

| Channels | #minetest-dev index | Today | | Google Search | Plaintext

All times shown according to UTC.

Time Nick Message
00:11 paramat probably not
00:11 calcul0n joined #minetest-dev
00:12 kaeza joined #minetest-dev
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:33 calcul0n joined #minetest-dev
01:47 paramat ok thanks, no rush
02:02 Miner_48er joined #minetest-dev
02:22 Nezrok joined #minetest-dev
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:03 paramat joined #minetest-dev
03:11 turtleman joined #minetest-dev
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:47 Wuzzy joined #minetest-dev
03:50 ssieb joined #minetest-dev
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:13 Foz joined #minetest-dev
04:22 sofar maybe not in minetest_game then
05:04 ANAND joined #minetest-dev
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:32 jas_ joined #minetest-dev
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:39 Cornelia joined #minetest-dev
05:41 paramat ok
05:49 Lia joined #minetest-dev
06:25 ichoquo0Aigh9ie joined #minetest-dev
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?
07:54 ichoquo0Aigh9ie joined #minetest-dev
08:24 proller joined #minetest-dev
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 <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:01 Beton joined #minetest-dev
10:03 rubenwardy I'm not sure whether removing the distinction between short and long strings is a good idea
10:55 Fixer joined #minetest-dev
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:24 YuGiOhJCJ joined #minetest-dev
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:16 entuland joined #minetest-dev
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"
13:32 entuland joined #minetest-dev
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 proller joined #minetest-dev
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:16 troller joined #minetest-dev
14:17 Megaf_ joined #minetest-dev
14:24 ensonic joined #minetest-dev
14:42 rubenwardy merging trivial bug fix in 10: https://github.com/rubenwardy/minetest/commit/c735497a65c8133c0df6b7ee7fc87dc4725994e6
14:54 Beton joined #minetest-dev
14:55 kaeptmblaubaer joined #minetest-dev
15:06 entuland joined #minetest-dev
15:27 entuland joined #minetest-dev
15:47 proller joined #minetest-dev
15:56 kaeza joined #minetest-dev
15:57 rubenwardy nore, sofar:  it would be good to hear your input on #8331 vs #8330
15:57 test123 joined #minetest-dev
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:08 proller joined #minetest-dev
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
16:49 kaeptmblaubaer joined #minetest-dev
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 entuland joined #minetest-dev
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:17 kaeptmblaubaer joined #minetest-dev
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:50 p_gimeno joined #minetest-dev
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:09 kaeza joined #minetest-dev
18:36 YuGiOhJCJ joined #minetest-dev
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
19:16 GreenDimond joined #minetest-dev
19:19 ssieb joined #minetest-dev
19:22 Fixer_ joined #minetest-dev
19:28 Fixer joined #minetest-dev
19:36 proller joined #minetest-dev
20:00 entuland joined #minetest-dev
20:03 diemartin joined #minetest-dev
20:12 Wuzzy joined #minetest-dev
20:46 nerzhul lol:)
20:48 rubenwardy #9999
20:48 p_gimeno https://github.com/minetest/minetest/issues/9999 -- Page not found &middot; 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:16 kaeza joined #minetest-dev
21:29 argyle77 joined #minetest-dev
21:32 ShadowBot joined #minetest-dev
21:32 p_gimeno ooh
21:33 ShadowBot joined #minetest-dev
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 ~<o<~
21:33 GreenDimond wat
21:34 ShadowBot GreenDimond: Error: Missing ">".  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
21:54 proller joined #minetest-dev
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
22:27 Player-2 joined #minetest-dev

| Channels | #minetest-dev index | Today | | Google Search | Plaintext