`%*` DO-argument expansion bug in scp.c::sim_sub_args()

[pmetzger]

Report via Codex, hand verified by me.

Summary

The %* expansion path in sim_sub_args is broken.

It has two problems:

1. the buffer-space check is inverted, so ordinary %* expansion usually
   produces an empty string instead of the remaining DO arguments
2. the quoted-argument format string appends an extra stray " at the end

User-visible behavior

The documented behavior is that %* expands to the whole remaining set of
DO-file arguments (%1 ... %9).

In practice, the current code usually stops immediately and appends nothing.

A minimal repro on the old code is just a tiny DO script.

Create star.do with:

echo 1=<%1>
echo 2=<%2>
echo *=<%*>

Then in a simulator:

sim> do star.do alpha beta

What a user would expect:

1=<alpha>
2=<beta>
*=<alpha beta>

What the buggy old code produces:

1=<alpha>
2=<beta>
*=<>

So %1 and %2 work, but %* expands to an empty string in an ordinary case.

Root cause

The relevant code in sim_sub_args currently does:

if ((sizeof(rbuf)-strlen(rbuf)) < (2 + strlen(do_arg[i]))) {
    ...
    sprintf(&rbuf[strlen(rbuf)], ...);
}
else
    break;

That condition is backwards.

It appends only when the remaining buffer space is smaller than the
required size, and stops when there is enough room.

So:

• normal inputs with adequate buffer space do not expand
• the append path is reached only in the too-small-buffer case

There is also a quoting typo in the spaced-argument branch:

"%s%c%s%c\""

which emits an extra trailing " after the closing quote.

Why this should be fixed

This is a correctness bug because %* does not perform the documented
expansion.

It is also a safety concern, because the only way into the append path is the
insufficient-space case, and that path uses sprintf into the remaining tail
of rbuf.

Patch

This patch is against the older monolithic scp.c layout:

diff --git a/scp.c b/scp.c
--- a/scp.c
+++ b/scp.c
@@ -4850,14 +4850,14 @@ void sim_sub_args (char *instr, size_t instr_size, char *do_arg[])
                         if (do_arg[i] == NULL)
                             break;
                         else
-                            if ((sizeof(rbuf)-strlen(rbuf)) < (2 + strlen(do_arg[i]))) {
+                            if ((sizeof(rbuf)-strlen(rbuf)) >= (2 + strlen(do_arg[i]))) {
                                 if (strchr(do_arg[i], ' ')) { /* need to surround this argument with quotes */
                                     char quote = '"';
                                     if (strchr(do_arg[i], quote))
                                         quote = '\'';
-                                    sprintf(&rbuf[strlen(rbuf)], "%s%c%s%c\"", (i != 1) ? " " : "", quote, do_arg[i], quote);
+                                    sprintf(&rbuf[strlen(rbuf)], "%s%c%s%c", (i != 1) ? " " : "", quote, do_arg[i], quote);
                                     }
                                 else
                                     sprintf(&rbuf[strlen(rbuf)], "%s%s", (i != 1) ? " " : "", do_arg[i]);
                                 }
                             else

Suggested regression test

Add a test that runs %* with ordinary DO arguments and checks the exact
expanded string, for example:

%1 = alpha
%2 = beta

and verify:

A%*B  ->  Aalpha betaB

That catches the current failure immediately.

[markpizz]

Thanks for catching and reporting this.

[markpizz]

Once again, thanks for catching this problem.

However your proposed fix is only partially correct. Specifically you suggest that:

if ((sizeof(rbuf)-strlen(rbuf)) >= (2 + strlen(do_arg[i]))) {

would be appropriate. This is indeed mostly correct, BUT a more correct test would actually be:

if ((sizeof(rbuf)-strlen(rbuf)) >= (4 + strlen(do_arg[i]))) {

The value 4 covers the possible prefix of a space character and also leaves room for a NUL terminator besides the potential quote characters.

[pmetzger]

Sorry about that. My implementation at this point has diverged enough that my fix isn't quite right for your code; I was not really conveying what my code looks like when I reported it. I also have unit tests that cover all this code so I'm pretty sure I don't have the problem now, but again, my code no longer looks like SIMH.