lightningd: catch edits of config files *before* we're committed.

Another report, that we crash if it's edited.  We should check that too!

Reported-by: daywalker90
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

lightningd/configs.c

@@ -416,12 +416,41 @@ static size_t append_to_file(struct lightningd *ld,
 	return strcount(buffer, "\n") + 1;
 }
 
+static const char *grab_and_check(const tal_t *ctx,
+				  const char *fname,
+				  size_t linenum,
+				  const char *expected,
+				  char ***lines)
+{
+	char *contents;
+
+	contents = grab_file(tmpctx, fname);
+	if (!contents)
+		return tal_fmt(ctx, "Could not load configfile %s: %s",
+			       fname, strerror(errno));
+
+	/* These are 1-based! */
+	assert(linenum > 0);
+	*lines = tal_strsplit(ctx, contents, "\r\n", STR_EMPTY_OK);
+	if (linenum >= tal_count(*lines))
+		return tal_fmt(ctx, "Configfile %s no longer has %zu lines!",
+			       fname, linenum);
+
+	if (!streq((*lines)[linenum - 1], expected))
+		return tal_fmt(ctx, "Configfile %s line %zu changed from %s to %s!",
+			       fname, linenum,
+			       expected,
+			       (*lines)[linenum - 1]);
+	return NULL;
+}
+
 /* This comments out the config file entry or maybe replace one */
 static void configfile_replace_var(struct lightningd *ld,
 				   const struct configvar *cv,
 				   const char *replace)
 {
-	char *contents, **lines, *template;
+	char **lines, *template, *contents;
+	const char *err;
 	int outfd;
 
 	switch (cv->src) {
@@ -437,21 +466,11 @@ static void configfile_replace_var(struct lightningd *ld,
 		break;
 	}
 
-	contents = grab_file(tmpctx, cv->file);
-	if (!contents)
-		fatal("Could not load configfile %s: %s",
-		      cv->file, strerror(errno));
-
-	lines = tal_strsplit(contents, contents, "\r\n", STR_EMPTY_OK);
-	if (cv->linenum - 1 >= tal_count(lines))
-		fatal("Configfile %s no longer has %u lines!",
-		      cv->file, cv->linenum);
-
-	if (!streq(lines[cv->linenum - 1], cv->configline))
-		fatal("Configfile %s line %u changed from %s to %s!",
-		      cv->file, cv->linenum,
-		      cv->configline,
-		      lines[cv->linenum - 1]);
+	/* If it changed *now*, that's fatal: we already set it locally! */
+	err = grab_and_check(tmpctx, cv->file, cv->linenum, cv->configline,
+			     &lines);
+	if (err)
+		fatal("%s", err);
 
 	if (replace)
 		lines[cv->linenum - 1] = cast_const(char *, replace);
@@ -595,14 +614,12 @@ static bool dir_writable(const char *fname)
 /* Returns config file name if not writable */
 static const char *config_not_writable(const tal_t *ctx,
 				       struct command *cmd,
-				       const struct opt_table *ot)
+				       const struct configvar *oldcv)
 {
 	struct lightningd *ld = cmd->ld;
-	struct configvar *oldcv;
 	const char *fname;
 
 	/* If it exists before, we will need to replace that file (rename) */
-	oldcv = configvar_first(ld->configvars, opt_names_arr(tmpctx, ot));
 	if (oldcv && oldcv->file) {
 		/* We will rename */
 		if (!dir_writable(oldcv->file))
@@ -645,11 +662,32 @@ static struct command_result *json_setconfig(struct command *cmd,
 	assert(!(ot->type & OPT_MULTI));
 
 	if (!*transient) {
-		const char *fname = config_not_writable(cmd, cmd, ot);
+		const struct configvar *cv;
+		const char *fname;
+
+		cv = configvar_first(cmd->ld->configvars,
+				     opt_names_arr(tmpctx, ot));
+
+		fname = config_not_writable(cmd, cmd, cv);
 		if (fname)
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 					    "Cannot write to config file %s",
 					    fname);
+
+		/* Check if old config has changed (so we couldn't be able
+		 * to comment it out! */
+		if (cv && cv->file) {
+			const char *changed;
+			char **lines;
+
+			changed = grab_and_check(tmpctx,
+						 cv->file, cv->linenum,
+						 cv->configline,
+						 &lines);
+			if (changed)
+				return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+						    "%s", changed);
+		}
 	}
 
 	/* We use arg = NULL to tell callback it's only for testing */

tests/test_misc.py

@@ -4278,6 +4278,55 @@ def test_setconfig_access(node_factory, bitcoind):
     os.chmod(includedir, 0o700)
 
 
+def test_setconfig_changed(node_factory, bitcoind):
+    """Test that we correctly fail (not crash) if config file changed"""
+    l1 = node_factory.get_node(start=False)
+
+    netconfigfile = os.path.join(l1.daemon.opts.get("lightning-dir"), TEST_NETWORK, 'config')
+    with open(netconfigfile, 'w') as file:
+        file.write("min-capacity-sat=100")
+    l1.start()
+
+    assert l1.rpc.listconfigs(config="min-capacity-sat")['configs']['min-capacity-sat']['value_int'] == 100
+
+    # Change it underneath
+    with open(netconfigfile, 'w') as file:
+        file.write("#some comment\nmin-capacity-sat=100")
+
+    # This will fail.
+    with pytest.raises(RpcError, match=f'Configfile {netconfigfile} line 1 changed from min-capacity-sat=100 to #some comment!'):
+        l1.rpc.check("setconfig", config="min-capacity-sat", val=9999)
+    with pytest.raises(RpcError, match=f'Configfile {netconfigfile} line 1 changed from min-capacity-sat=100 to #some comment!'):
+        l1.rpc.setconfig(config="min-capacity-sat", val=9999)
+
+    # Restore it.
+    with open(netconfigfile, 'w') as file:
+        file.write("min-capacity-sat=100")
+
+    # Succeeds
+    l1.rpc.setconfig(config="min-capacity-sat", val=9999)
+
+    # Now mess with config.setconfig...
+    setconfigfile = netconfigfile + ".setconfig"
+    with open(setconfigfile, 'w') as file:
+        pass
+
+    # Now this will fail (truncated)
+    with pytest.raises(RpcError, match=f'Configfile {setconfigfile} no longer has 2 lines'):
+        l1.rpc.check("setconfig", config="min-capacity-sat", val=9999)
+    with pytest.raises(RpcError, match=f'Configfile {setconfigfile} no longer has 2 lines'):
+        l1.rpc.setconfig(config="min-capacity-sat", val=9999)
+
+    # This will fail (changed)
+    with open(setconfigfile, 'w') as file:
+        file.write("# Created and update by setconfig, but you can edit this manually when node is stopped.\nmin-capacity-sat=999")
+
+    with pytest.raises(RpcError, match=f'Configfile {setconfigfile} line 2 changed from min-capacity-sat=9999 to min-capacity-sat=999!'):
+        l1.rpc.check("setconfig", config="min-capacity-sat", val=9999)
+    with pytest.raises(RpcError, match=f'Configfile {setconfigfile} line 2 changed from min-capacity-sat=9999 to min-capacity-sat=999!'):
+        l1.rpc.setconfig(config="min-capacity-sat", val=9999)
+
+
 @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "deletes database, which is assumed sqlite3")
 def test_recover_command(node_factory, bitcoind):
     l1, l2 = node_factory.get_nodes(2)