[BUG] rpcd service does not process all states correctly

[remittor]

Test script:

rpctest.sh

#!/bin/sh

EXE_DIR=$( cd "$(dirname "$0")" 2>/dev/null && pwd )

RPC_URL="http://127.0.0.1/ubus"

usage() {
	echo "Usage:"
	echo "  $0 -p <password> -c <command> [-s]"
	echo
	echo "Options:"
	echo "  -p  root password"
	echo "  -c  command string (no quotes inside!)"
	echo "  -s  run via 'sh -c'"
	echo "  -T  run test with N iterations"
	exit 1
}

PASSWORD=""
CMDLINE=""
USE_SH=0
TEST_MAX=0

while getopts "p:c:sT:" opt; do
	case "$opt" in
		p) PASSWORD="$OPTARG" ;;
		c) CMDLINE="$OPTARG" ;;
		s) USE_SH=1 ;;
		T) TEST_MAX="$OPTARG" ;;
		*) usage ;;
	esac
done

[ "$PASSWORD" = "@" ] && PASSWORD=xxxxxxxxxxx
[ -z "$PASSWORD" ] && usage
[ -z "$CMDLINE" ] && usage

LOGIN_JSON=$( cat <<EOF
{
	"jsonrpc": "2.0",
	"id": 1,
	"method": "call",
	"params": [
		"00000000000000000000000000000000",
		"session",
		"login",
		{
			"username": "root",
			"password": "$PASSWORD"
		}
	]
}
EOF
)

LOGIN_REPLY=$( curl -s "$RPC_URL" -H 'Content-Type: application/json' -d "$LOGIN_JSON" )

SESSION=$( echo "$LOGIN_REPLY" | sed -n 's/.*"ubus_rpc_session"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' )

if [ -z "$SESSION" ]; then
	echo "ERROR: login failed"
	echo "$LOGIN_REPLY"
	exit 2
fi

if [ "$USE_SH" -eq 1 ]; then
	# sh -c "string"
	CMD="/bin/sh"
	PARAMS_JSON="\"-c\",\"$CMDLINE\""
else
	# split by spaces
	set -- $CMDLINE
	CMD="$1"
	shift
	PARAMS_JSON=""
	for a in "$@"; do
		PARAMS_JSON="$PARAMS_JSON,\"$a\""
	done
	PARAMS_JSON="${PARAMS_JSON#,}"
fi

EXEC_JSON=$( cat <<EOF
{
	"jsonrpc": "2.0",
	"id": 2,
	"method": "call",
	"params": [
		"$SESSION",
		"file",
		"exec",
		{
			"command": "$CMD",
			"params": [ $PARAMS_JSON ]
		}
	]
}
EOF
)

TEST_ITER=0
while true; do
	TEST_ITER=$((TEST_ITER + 1))
	TEST_RESP=$( curl -s "$RPC_URL" -H 'Content-Type: application/json' -d "$EXEC_JSON" )
	echo "$TEST_RESP"
	if ! echo "$TEST_RESP" | grep -q '"result":\[0' ; then
		echo "ERROR: response with error code. Iteration = $TEST_ITER"
		exit 1
	fi
	[ $TEST_ITER -ge $TEST_MAX ] && break
done

echo ""

After running the script, you need to wait for a collision to occur:

> ./rpctest.sh -p @ -s -c '( exec >/dev/null 2>&1 ; echo 123 ) & ' -T 3000000
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
.....
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
{"jsonrpc":"2.0","id":2,"result":[0,{"code":0}]}
{"jsonrpc":"2.0","id":2,"result":[7]}
ERROR: response with error code. Iteration = 2178

A full description of the cause of the error and a fix can be found here: https://forum.openwrt.org/t/245914

Fix: remittor-pr@57e08ec

[jow-]

Okay, so the problem your fix addresses is:

• When process ends, uloop disaptches a process callback (rpc_file_exec_process_cb())
• In the process end callback we knock both output and error pipes one more time (ustream_poll(&c->Xpipe.stream)), hoping it will trigger EOF reception which in turn will trigger the reply once both pipes reach EOF
• The ustream_poll() will trigger a read() which sometimes, instead of "reading" an EOF, will immediately fail with EAGAIN - ath this point we will not try reading EOF again, the process hangs in limbo until eventually the 30s timeout kicks in

Is my understanding right so far?

I'd like to simplify the pipe EOF probing retry logic but that requires another kind of related fix

[remittor]

Is my understanding right so far?

In general terms, it seems like you understood it correctly.

I'd like to simplify the pipe EOF probing retry logic but that requires another kind of related fix

Remember that when calling function ustream_poll you may get EAGAIN, but you will not know about this condition.

[jow-]

After e655a0d ("exec: defer async reply teardown to avoid use-after-free") we should be able to simplify the fix into just this:

diff --git a/file.c b/file.c
index eeb0713..8203f82 100644
--- a/file.c
+++ b/file.c
@@ -849,8 +849,11 @@ rpc_file_exec_process_cb(struct uloop_process *p, int stat)
 
        c->stat = stat;
 
-       ustream_poll(&c->opipe.stream);
-       ustream_poll(&c->epipe.stream);
+       do {
+               ustream_poll(&c->opipe.stream);
+               ustream_poll(&c->epipe.stream);
+       }
+       while (!c->opipe.stream.eof || !c->epipe.stream.eof);
 }
 
 static void

[remittor]

Well, this code relies on the fact that the EOF will be received immediately after EAGAIN, without delay.
Or maybe you just forgot that an asynchronous approach to writing code is required here.