json-rpc: translate NULL into "null" instead of "(null)".

[bitonic-cjp]

It turns out that #957 is insufficient to solve #908 . This fix is needed to really say "id": null instead of "id": (null).

This is probably an incomplete fix: it would be better to make sure that all cases of NULL are translated into "null". However, I didn't want to touch the tal_fmt implementation. What would be a good approach? make a json_fmt wrapper around tal_fmt to do this? I didn't do this yet: since I'm not too familiar with the code base yet, I didn't want to make too large changes without any guidance.

[ZmnSCPxj]

tal_fmt is a wrapper around vsnprintf, and if you want to be pedantic, a "%s" format string should not be given a NULL as that will lead to undefined behavior. It is simply that the behavior in glibc is to print "(null)" in that case; we cannot rely on all systems doing that.

[ZmnSCPxj]

I believe this is the point which we should modify:

lightning/lightningd/jsonrpc.c

Lines 687 to 689 in 73cda2f

	json_command_malformed(
	jcon, NULL,
	"Invalid token in json input");

It should be json_command_malformed(jcon, "null", "Invalid token in json input");

Then inside both connection_complete_error and connection_complete_ok we should have assert(id != NULL) to catch this in the future.

[bitonic-cjp]

Agreed: that seems to be somewhat less ugly than what I made.

Could we somehow ensure that NULL is never passed to tal_fmt through another route? I find it scary to have stumbled upon something that is actually undefined behavior.

[ZmnSCPxj]

Wait for @practicalswift to do regular undefined behavior checking has always been my personal technique, hahaha.

[rustyrussell]

Was about to suggest the same as @ZmnSCPxj : let's kill the root cause, and assert() that it never happens again.

[ZmnSCPxj]

On a more serious note, @bitonic-cjp: undefined behavior is mostly caught by code review and sometimes by automated tools that scan for them, and valgrind can catch some bits of undefined behavior related to usage of memory. @practicalswift might have a tool or two that scans for undefined behavior (and has requested pulls to add such linting to the python parts of our codebase, for instance). I have caught a few bugs myself (e.g. subdaemons blocking SIGCHLD when they should be blocking SIGPIPE, waitanyinvoice blocking even with an invoice to report or reporting an invoice twice) simply from code review. So mostly.... with enough eyes, all bugs are shallow. Hopefully with more people supporting development we can get more bugs found and fixed.

[bitonic-cjp]

I did what ZmnSCPxj said; now I'm checking other tal_fmt calls.

[ZmnSCPxj]

ACK 59b7f9e

[practicalswift]

Nit: Make sure the indentation matches the surrounding code. Same below.

[bitonic-cjp]

Agreed; I already discovered this myself, and was going to fix it.

[cdecker]

ACK 500db24