| Time |
Nick |
Message |
| 02:39 |
|
Seirdy joined #minetest-dev |
| 04:40 |
MTDiscord |
<Lone_Wolf> Bump https://github.com/minetest/minetest/pull/10449 |
| 05:57 |
|
NetherEran joined #minetest-dev |
| 06:24 |
|
NetherEran joined #minetest-dev |
| 07:28 |
|
Icedream joined #minetest-dev |
| 08:00 |
|
ShadowNinja joined #minetest-dev |
| 09:26 |
|
Wuzzy joined #minetest-dev |
| 09:36 |
|
lisac joined #minetest-dev |
| 09:43 |
|
calcul0n joined #minetest-dev |
| 10:40 |
|
troller joined #minetest-dev |
| 10:44 |
|
Fixer joined #minetest-dev |
| 10:54 |
|
pmp-p joined #minetest-dev |
| 11:46 |
|
calcul0n joined #minetest-dev |
| 13:02 |
|
NetherEran joined #minetest-dev |
| 13:24 |
MTDiscord |
<appguru> base64 is still wrong Krock / SmallJoker |
| 13:24 |
MTDiscord |
<appguru> fixing it |
| 13:37 |
MTDiscord |
<appguru> https://github.com/minetest/minetest/pull/10515 |
| 13:56 |
Krock |
though currently it works |
| 13:56 |
Krock |
perhaps just inaccurate |
| 14:02 |
MTDiscord |
<appguru> well, it has false positives |
| 14:02 |
MTDiscord |
<appguru> "A=A" is invalid base64, but would be accepted by the current implementation |
| 14:03 |
MTDiscord |
<appguru> on a sidenote, my fix should increase performance as there is no need to check every character against = |
| 14:07 |
Krock |
there would only be a single '=' comparison in the whole process because if previous logical OR conditions evaluate to "true", the other checks are skipped |
| 14:11 |
MTDiscord |
<appguru> true, but I save you the "or" |
| 14:12 |
rubenwardy |
The performance increase will be negligible |
| 14:13 |
rubenwardy |
A fix to correct behaviour would be the benefit |
| 14:14 |
MTDiscord |
<appguru> yes |
| 14:15 |
sfan5 |
please don't put that unrelated change in there |
| 14:18 |
MTDiscord |
<appguru> ok |
| 14:18 |
pgimeno |
base64 has been gotten wrong three times by the engineers at Linden Lab; pay close attention to the implementation and test it thoroughly and with invalid cases, like a string starting with a non-base64, of with a =, or with a character and a =, or with a character and an invalid symbol, etc. |
| 14:20 |
MTDiscord |
<appguru> I'm pretty certain that my fix works |
| 14:20 |
pgimeno |
for XORing in particular: first we had llXorBase64Strings, then llXorBase64StringsCorrect, then llXorBase64, and that one also had a bug that got caught early enough (but now they have to live with the other two because of compatibility) |
| 14:28 |
pgimeno |
I may be missing something, but I don't see a check for i % 4 |
| 14:28 |
pgimeno |
this base64 string is invalid, but it seems to be accepted: A= |
| 14:29 |
pgimeno |
this one is wrong too: ABCDE== |
| 14:29 |
pgimeno |
basically, if number of base64 chars % 4 = 1, the string is wrong |
| 14:30 |
sfan5 |
the code should calculate and check the amount of required padding |
| 14:33 |
MTDiscord |
<appguru> Well, we allow omitting padding |
| 14:33 |
pgimeno |
it could be more permissive with what it accepts, and accept strings without padding signs, but then care must be taken to avoid the last character if the total length % 4 = 1, so that e.g. a base64 string with 1 base64 char, optionally followed by padding, is taken as an empty string |
| 14:33 |
MTDiscord |
<appguru> We could check that IF padding is used, it has to be proper |
| 14:34 |
sfan5 |
the simplified check is fine then |
| 14:34 |
pgimeno |
in that case, every string is valid base64 |
| 14:34 |
pgimeno |
no need to check it |
| 14:34 |
rubenwardy |
This is the sort of code that can be unit tested very easily |
| 14:34 |
pgimeno |
yes, but first a specification is needed |
| 14:35 |
MTDiscord |
<appguru> True, and it should be documented in lua_api.txt |
| 14:35 |
MTDiscord |
<appguru> We have to keep allowing the omission of padding |
| 14:35 |
MTDiscord |
<appguru> How we deal with padding remains the question |
| 14:37 |
pgimeno |
for rigorous base64, where two base64 strings are equal iff the binary strings they represent are equal, additional checks need to be done on the last character in case i % 4 = 2 or 3 |
| 14:37 |
pgimeno |
I'd vote for "any string is a base64 string" |
| 14:38 |
pgimeno |
i.e. don't check padding, just consume base64 chars and omit the last if the total number of them % 4 = 1 |
| 14:39 |
pgimeno |
(stopping only at the first non-base64, or at end of string) |
| 14:39 |
sfan5 |
I don't know why you are making this so complicated, base64 decoding currently allows = in the middle of a string despite being invalid |
| 14:39 |
pgimeno |
and doesn't that stop the decoding? |
| 14:39 |
sfan5 |
and this PR changes it to only accept = at the end, which is closer to what it should do |
| 14:39 |
sfan5 |
I have no idea but that doesn't matter because it isn't supposed to decode invalid base64 |
| 14:40 |
MTDiscord |
<appguru> It decodes invalid base64 anyways tho |
| 14:40 |
sfan5 |
¯\_(ツ)_/¯ |
| 14:40 |
MTDiscord |
<appguru> From a glance it appears it stops if it encounters padding? |
| 14:41 |
MTDiscord |
<appguru> while (in_len-- && ( encodedstring[in] != '=') && is_base64(encodedstring[in])) |
| 14:41 |
pgimeno |
I'm not making it complicated at all, just bool base64_is_valid(std::string const& s) { return true; } |
| 14:41 |
pgimeno |
and for the decoder, the idea is to stop at the first break in a loop similar to this: https://github.com/minetest/minetest/pull/10515/commits/8409aef3f2bb5af295bb131a9977c5924ebc4dc3 |
| 14:41 |
pgimeno |
it's actually the simplest |
| 14:41 |
MTDiscord |
<appguru> The decoder already does it |
| 14:41 |
pgimeno |
just additionally do: if (i % 4 == 1) --i; |
| 14:42 |
sfan5 |
then replace "complicated" with "nonsense" |
| 14:42 |
MTDiscord |
<appguru> Yeah, such an implementation will lead to strange bugs |
| 14:42 |
sfan5 |
this PR improves base64 validation and your suggestion is "it should attempt to decode whatever garbage the user gives in, php style" |
| 14:42 |
MTDiscord |
<appguru> Also, people might use decode_base64 to validate it |
| 14:42 |
MTDiscord |
<appguru> "returns string or nil for invalid base64" - lua_api.txt |
| 14:43 |
pgimeno |
sfan5: well, if you want a mid term (not rigorous base64 but not relaxed), then you need to define what you want |
| 14:43 |
MTDiscord |
<appguru> base64_is_valid is currently not exposed |
| 14:43 |
MTDiscord |
<appguru> I'd say we go with optional padding, but if padding is used, check whether it's right |
| 14:43 |
MTDiscord |
<appguru> Note: My PR doesn't check the amount of padding characters, it just ensures it's between 0 and 2 at the end |
| 14:44 |
MTDiscord |
<appguru> Let me add this... |
| 14:44 |
sfan5 |
pgimeno: true and I think "base64 with or without padding" is a good enough specification |
| 14:46 |
pgimeno |
here's my suggestion for a mid term: construct a base64_is_valid that checks for rigorous base64 including pad bits as well as padding chars, and expose it to Lua; and a base64 decoder that accepts anything |
| 14:46 |
Krock |
or keep base64_is_valid as fast sanity check prior passing it to the real decoder |
| 14:47 |
MTDiscord |
<appguru> In that case you could save the is_base64 check in the decoder loop BTW |
| 14:48 |
pgimeno |
yeah, that's my point, it's not necessary and since everything is a base64 string, the decoder is not penalized |
| 14:48 |
pgimeno |
it just needs to stop at the first base64 character |
| 14:48 |
pgimeno |
non* base64 |
| 14:49 |
pgimeno |
user code that cares about the well-formedness of the string, can check with the exposed lua check; user code that doesn't, can just throw it into the decoder |
| 14:51 |
MTDiscord |
<appguru> hmm, padding should be base64 size % 4 ? |
| 14:51 |
pgimeno |
padding should make the total string length % 4 = 0 |
| 14:52 |
pgimeno |
so, 3-size%4 |
| 14:53 |
MTDiscord |
<appguru> yes |
| 14:53 |
MTDiscord |
<appguru> thanks |
| 14:54 |
pgimeno |
sorry, 3-(size+3)%4 |
| 14:58 |
|
appguru joined #minetest-dev |
| 14:58 |
pgimeno |
additionally, for it to be a canonical base64 string, when i % 4 = 2, the last character can only be one of [AQgw], and when i % 4 = 3, one of [AEIMQUYcgkosw048] |
| 14:58 |
pgimeno |
see https://www.ietf.org/rfc/rfc4648.txt section 3.5 |
| 15:00 |
appguru |
That document uses another alphabet |
| 15:01 |
pgimeno |
what? is Minetest not using A-Za-z0-9+/ ? |
| 15:01 |
appguru |
Sorry my fault |
| 15:02 |
appguru |
The alphabet is right |
| 15:02 |
appguru |
Was looking at the safe alphabet... |
| 15:12 |
appguru |
alright, done |
| 15:13 |
appguru |
now I'm not so sure anymore, it will need testing... |
| 15:13 |
rubenwardy |
by unit tests |
| 15:14 |
rubenwardy |
https://github.com/minetest/minetest/blob/master/src/unittest/test_utilities.cpp |
| 15:21 |
appguru |
Exposed it to the Lua API now |
| 15:29 |
pgimeno |
http://www.formauri.es/personal/pgimeno/pastes/valid64.lua |
| 15:32 |
pgimeno |
reload for more test cases |
| 15:33 |
pgimeno |
consider that file public domain |
| 15:34 |
MTDiscord |
<appguru> hmm nice |
| 15:34 |
pgimeno |
anyway, appguru, I'm not a code developer, I can only give advice, so better do as sfan5 says |
| 15:34 |
pgimeno |
code -> core |
| 16:21 |
|
lisac joined #minetest-dev |
| 17:35 |
|
homthack joined #minetest-dev |
| 17:40 |
|
calcul0n_ joined #minetest-dev |
| 17:58 |
|
Seirdy joined #minetest-dev |
| 18:18 |
Krock |
#10510 can this be kept close or shall I re-open it? |
| 18:18 |
pgimeno |
https://github.com/minetest/minetest/issues/10510 -- Switching backend |
| 18:18 |
ShadowBot |
Krock: Error: That URL appears to have no HTML title within the first 4KB. |
| 18:18 |
Krock |
<3 pgimeno |
| 18:19 |
|
lisac joined #minetest-dev |
| 18:19 |
pgimeno |
a temporary solution while the bot is fixed :) |
| 18:19 |
Krock |
I love it. |
| 18:23 |
Krock |
sfan5: would it be possible that you meant "position vector" or "vector" here? https://github.com/minetest/minetest/pull/10512#discussion_r507188051 |
| 18:29 |
|
Fixer joined #minetest-dev |
| 18:41 |
|
mizux joined #minetest-dev |
| 18:42 |
sfan5 |
I meant to draw attention to the fact that it still says "tables" |
| 18:53 |
Krock |
ah. to me it looked like a suggestion |
| 18:54 |
|
lisac joined #minetest-dev |
| 19:06 |
|
fluxflux joined #minetest-dev |
| 19:10 |
|
lisac joined #minetest-dev |
| 19:18 |
|
lisac joined #minetest-dev |
| 19:40 |
|
Taoki joined #minetest-dev |
| 20:26 |
|
DS-minetest joined #minetest-dev |
| 20:54 |
|
DS-minetest joined #minetest-dev |
| 21:27 |
|
paramat joined #minetest-dev |
| 21:28 |
paramat |
merging trivial #10520 in 15 mins. will merge fairly trivial #10493 in 24 hrs if no objections |
| 21:28 |
ShadowBot |
paramat: Error: That URL appears to have no HTML title within the first 4KB. |
| 21:28 |
pgimeno |
https://github.com/minetest/minetest/pull/10520 -- Contributing doc: Minor improvements and a clarification by paramat |
| 21:28 |
ShadowBot |
paramat: Error: That URL appears to have no HTML title within the first 4KB. |
| 21:28 |
pgimeno |
https://github.com/minetest/minetest/pull/10493 -- Devtest: Automatically enable zoom capability by paramat |
| 21:29 |
paramat |
thanks pgibot |
| 22:10 |
|
DS-minetest joined #minetest-dev |
| 22:16 |
|
Hecklechet joined #minetest-dev |
| 22:17 |
|
Hecklechet left #minetest-dev |
| 22:22 |
pgimeno |
lol |
