digiline NIC crash

[OgelGames]

2020-09-14 13:09:53: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:10:54: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:11:42: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)
2020-09-14 13:11:54: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)

2020-09-14 13:14:19: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:15:05: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)
2020-09-14 13:15:07: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)
2020-09-14 13:15:19: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:15:38: ERROR[CurlFetch]: https://subgames.minetest.land/s.php?pw=( )&i=spider$0$0$0 not found (HTTP response code said error) (response code 400)
2020-09-14 13:16:19: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:17:06: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)
2020-09-14 13:17:19: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=  not found (HTTP response code said error) (response code 400)
2020-09-14 13:17:25: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)
2020-09-14 13:17:34: ERROR[CurlFetch]: https://subgames.minetest.land/r.php?pw=( )&i=spider not found (HTTP response code said error) (response code 400)
2020-09-14 13:17:37: ERROR[CurlFetch]: https://subgames.minetest.land/g.php?pw=( ) not found (HTTP response code said error) (response code 400)

EDIT: found what was causing it, it was this code in a Lua controller connected to an NIC: (there is actually a lot more, but this is the bit that is causing the crashes:

function write(name, pos)
 digiline_send("nic", "https://subgames.minetest.land/s.php?pw="..mem.pw.."&i="..name.."$"..pos.x.."$"..pos.y.."$"..pos.z)
end
function load()
 digiline_send("nic", "https://subgames.minetest.land/g.php?pw="..mem.pw)
end
function delete(name)
 digiline_send("nic", "https://subgames.minetest.land/r.php?pw="..mem.pw.."&i="..name)
end

[BuckarooBanzay]

interesting, the request itself should not crash the server.. 🤔
@Lejo1 this looks like your code 😄 any ideas?

[Lejo1]

Hmm,

this Lua controller saves and reads data to my Webserver. I have multiple of them so they will sync automatically the coordinates for the jumpdrives but it never caused a crash and I also wasn't online the last time.

Were there any changes to the mod or maybe to the minetest release?

[BuckarooBanzay]

Were there any changes to the mod or maybe to the minetest release?

Not that i'm aware of 🤷 there might be more details in the log files, i'll check them later today, thanks for the reply 👍

[BuckarooBanzay]

disabled the NIC until i can replicate/solve that issue, i hope nothing breaks because of the unknown node 😓

[S-S-X]

disabled the NIC until i can replicate/solve that issue, i hope nothing breaks because of the unknown node

Could also just override NIC functionality with noop() via pandorabox customizations to keep nodes in world but functionality disabled.
I mean like this:

minetest.override_item("digistuff:nic",{ digiline = { receptor = {}, effector = {} } })

[BuckarooBanzay]

I was in a hurry, commenting out from secure.http_mods seemed like the obvious choice at the time...
Anyways: i applied your changes here: pandorabox-io/pandorabox_custom@6aa53d8

EDIT: there were no additional infos in the debug logs, i have no idea how to reproduce this

[S-S-X]

sorry for that.. misclicked..

Actually I was thinking could this be just bad luck and server just caused segfault for whatever another reason and nic errors just happened to be there just before it... of course very hard to say for sure without any backtrace.

[BuckarooBanzay]

sorry for that.. misclicked..

Actually I was thinking could this be just bad luck and server just caused segfault for whatever another reason and nic errors just happened to be there just before it... of course very hard to say for sure without any backtrace.

I thought that too at first but there were no segfaults 😕

[OgelGames]

So I did some testing with the code I copied, and I was able to recreate the error, but it didn't crash, I just got the errors in chat 😕

I have the full code if you want it (all 251 lines of it), but this is all I needed to produce the errors: (note the single space after pw=)

digiline_send("nic", "https://subgames.minetest.land/r.php?pw= ")

It seems that the cause of the problem is that the NIC doesn't filter spaces, and just leaves them as is when sending the request, because any text after pw= that doesn't contain spaces works as intended.

It still doesn't explain why it crashed though, I might try it on the test server to see if it crashes there...

EDIT: no crashes on test server 😕

[OgelGames]

I changed the override in pandorabox_custom to add in a simple gsub replacement for spaces to fix the issue temporarily, it really should be fixed in digistuff, but until then this will restore the nic functionality.

[S-S-X]

@OgelGames should there now also be guard against msg that is not string? gsub probably wont like tables for example

[OgelGames]

should there now also be guard against msg that is not string?

Yes there should, totally forgot about that...

EDIT: done: pandorabox-io/pandorabox_custom@3916c3b

[S-S-X]

Just in case I did ran simple test, tried to feed some weird url characters:

local http = minetest.request_http_api()

local function t(msg)
        http.fetch(
                {url = msg,timeout = 1,user_agent = "Minetest Digilines Modem",},
                function(res)
                        print(string.format("Result: %s", dump(res.code)))
                end
        )
end

minetest.after(3, function()
for i=0,255,1 do
        for j=0,255,1 do
                t("http://127.0.0.1/" .. string.char(i) .. " " .. string.char(j))
        end
end
end)

At least this did not crash. Some codes were zero instead of valid HTTP status, didnt check but pretty sure http.fetch refused to handle some control chracters and returned zero code instead.
edit. also ran same thing again but weirdo chars in query string.

[OgelGames]

Some codes were zero instead of valid HTTP status, didnt check but pretty sure http.fetch refused to handle some control chracters and returned zero code instead.

Hmm... Maybe the http api should be the one checking for spaces, because spaces should never be in a url...

[S-S-X]

Maybe the http api should be the one checking for spaces

You should then build complete list of invalid characters. Not sure what this check should do, should it just fail like it did with some URLs I tried? That could maybe make it better but might also break other things if it is already used with some not so strict things.
However docs are out there (actually down there), HTTP API could also encode characters but that would cause problems.

https://tools.ietf.org/html/rfc3986#section-2.2
RFC 3986 URI Generic Syntax January 2005

2.2. Reserved Characters

URIs include components and subcomponents that are delimited by
characters in the "reserved" set. These characters are called
"reserved" because they may (or may not) be defined as delimiters by
the generic syntax, by each scheme-specific syntax, or by the
implementation-specific syntax of a URI's dereferencing algorithm.
If data for a URI component would conflict with a reserved
character's purpose as a delimiter, then the conflicting data must be
percent-encoded before the URI is formed.

  reserved    = gen-delims / sub-delims

  gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"

  sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
              / "*" / "+" / "," / ";" / "="

So, all characters above must be left as is in HTTP API and user should handle encoding if those are not used as delimiters.

2.3. Unreserved Characters

Characters that are allowed in a URI but do not have a reserved
purpose are called unreserved. These include uppercase and lowercase
letters, decimal digits, hyphen, period, underscore, and tilde.

  unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

URIs that differ in the replacement of an unreserved character with
its corresponding percent-encoded US-ASCII octet are equivalent: they
identify the same resource. However, URI comparison implementations
do not always perform normalization prior to comparison (see Section
6). For consistency, percent-encoded octets in the ranges of ALPHA
(%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E),
underscore (%5F), or tilde (%7E) should not be created by URI
producers and, when found in a URI, should be decoded to their
corresponding unreserved characters by URI normalizers.

Not sure if HTTP API should handle any urlencoding, it would enforce correct syntax if done for limited set of characters but it will be impossible to make correct decisions when reserved characters should be encoded and when those should not be encoded. Should API then do any of that or leave all encoding for user (digistuff:nic code) to do?

I think encoding some and leaving some unencoded would make HTTP API too complex and would also require a lot more documentation + user must then make sure that double encoding wont happen.

Possible problem if HTTP API fails when called with URI that is invalid by RFC spec but handled correctly by (insert service here).
Main problem if HTTP API would do encoding is double encoding.

Should probably dive a bit deeper into most important use cases to start making decisions suggestions about what HTTP API can clean up or refuse to handle, it will probably get more complicated after some digging. Ofc could still try to open ticket for this, probably there's people who know a bit more about this stuff.

But here I think digistuff:nic as limited application using HTTP API could handle encoding for some special characters or clean up / refuse to pass URLs containing some characters to http.fetch.

Multiple choices, not sure what would be best. If you whoever had time to read that wall of text maybe you got some ideas why something should or should not be done.

[S-S-X]

NIC / HTTP API seems to send spaces and many other characters as is, many web servers will return HTTP 400 but this for example kinda works when requesting URL "http://127.0.0.1/ .html":

printf 'HTTP/1.1 200 OK\n\nHelloWorld' | sudo nc -l 80

Results from http.fetch:

{
        succeeded = false,
        code = 200,
        completed = true,
        timeout = true,
        data = "HelloWorld"
}

Note succeeded was false even while response got through and code is 200.

NIC seems to work correctly fulfilling user request and also responds with actual error code returned from actual web server (one that wont handle spaces in URL):

{
        succeeded = false,
        code = 400,
        completed = true,
        timeout = false,
        data = ""
}

This causes error message to be visible in logs but that should not make any real harm, not sure if this actually should be logged as error. Warning would make more sense for me as this is still completely normal and correct response:

ERROR[CurlFetch]: http://127.0.0.1/ .html not found (HTTP response code said error) (response code 400)

I don't think anything should be changed in HTTP API itself, it does what user requests and seems to handle it correctly.
However it could be that crash was still caused because response contained some stuff that HTTP API could not handle

Would need to dive into actual HTTP API implementation to test this case. What it tries to do with response and is there something (in code) that could explain crash when response contains some weird stuff.

edit. just for completeness here's also raw request sent by NIC when URL contains "invalid" character (space):

GET / .html HTTP/1.1
Host: 127.0.0.1
User-Agent: Minetest Digilines Modem
Accept: */*
Accept-Encoding: gzip