Steps to reproduce and observed behaviour:
$ cat t.sh
#!/bin/sh
echo $#
sleep 5
$ ./t.sh {0..999}
1000
$ screen ./t.sh {0..999}
[ displays 1000 and exits ]
$ screen screen ./t.sh {0..999}
[ displays 62 and exits ]
$
Expected behaviour: 1000 in all cases.
Tolerable behaviour:
1000 where currently supported. Where not supported, a fatal error
(message on stderr plus nonzero exit status).
This is a potential data loss bug so might justify even "grave" but I
thought that was overkill. I hope you will agree that this deserves
an RC bug.
This has been reported upstream here:
https://savannah.gnu.org/bugs/?61504
Ian.
Also, it has a problem with long arguments:
$ cat t.sh
#!/bin/sh
all="$*"
echo $# ${#all}
sleep 5
$ ./t.sh a "$(yes | head -10000)" b
3 20003
$ screen ./t.sh a "$(yes | head -10000)" b
[ displays "3 20003", pauses 5 seconds, exits ]
$ screen screen ./t.sh a "$(yes | head -10000)" b
[ displays "1 1", pauses 5 seconds, exits ]
$
Here it loses all the arguments after the first long one !
I grepped for MAXARGS and arranged for screen to crash rather than silently ignoring arguments. I think this is much better, although it doesn't fully fix the bug. Confusingly, the source uses MAXARGS both for the maximum number of arguments for a shell command, and for the maximum number of numerical arguments to a terminal escape sequence. I left the latter part alone: excess terminal escape sequence arguments are still ignored. Applying this patch wouldn't actually fix the bug but it would IMO render it of "normal" or "minor" severity. Ian.
Hi Ian, thanks for forwarding this issue. I meant to reply quite a while ago, but in the end seem to have forgotton to actually send it. Most of it was written already in the beginning of December. Ian Jackson wrote: So this issue basically solely exists at startup. Or possibly if someone types in more than 62 parameters manually at "Ctrl-A :screen" (which I can't imagine that it really happens). I thought about that a while (but didn't come to a conclusion quickly, so I only started a few weeks after the bug report), but while I agree that this is "data loss", the actual impact of this issue seems so small that it IMHO does not deserve RC status. I'm also not really happy with the attached patch (from your second mail) and still undecided if I should really apply a patch that deliberately causes a crash. Then again, it might just be poorly named as it does not seem to cause a crash (like a segmentation fault), just exiting in panic. I'm removing the patch tag as the patch doesn't fix the issue but just makes it more visible. Marked as forwarded, thanks. (Already did that back then after I saw the bug report.) Also note that upstream marked the bug report in Savannah as confirmed as well. So there's hope that upstream finds a proper patch for it. Regards, Axel
Axel Beckert writes ("Re: Bug#1000138: /usr/bin/screen: command arguments beyond 62 silently discarded"):
Fair enough.
Yes, by "crash" I didn't mean segfault.
Could you apply the patch in the meantime, though ? It is definitely
an improvement.
FYI, this bug really happened to a friend of mine, who was doing sme
large numerical processing tasks. They were constructing the screen
command line with a shell glob. The result was that some of their job
mysteriously simply didn't occur. This was confusing and my friend
spent some time poring over their Makefiles etc.
In another situation, the user might not have noticed, and might have
proceeded with outputs that were only calculated from some of the
inputs.
I think bailing completely is nearly always better than silently
forgetting half the job. This is why I felt this bug is RC: it can
cause data loss, and came quite close for my friend. With my patch it
is simply a bug, and one which the user can then work around.
Thanks,
Ian.
Hi Ian, Ian Jackson wrote: I'm currently preparing an upload of the new upstream release 4.9.0. It doesn't contain a fix for that, but upstream hinted that they might include a fix in the next release — which I guess will become 4.9.1: https://lists.gnu.org/archive/html/screen-devel/2022-01/msg00020.html So I applied the patch to 4.9.0 and tested the result as documented in this bug report, see the branch debian-bug-1000138 on Salsa: https://salsa.debian.org/debian/screen/-/tree/debian-bug-1000138 Unfortunately the relevant error message "Argument count or length limit exceeded" does not show up with the patch. The only thing I see when the bug is triggered with the patch is "[screen is terminating]" immediately (instead of waiting the five seconds given in the script). So I won't include it for now, but I'm open to include a patch if it outputs a visible error message to the end-user. But I actually do hope that upstream will have a proper fix soon for this issue. I'd also include an upstream fix in the package e.g. if the next release isn't ready yet, but a patch already available in the upstream git repository. Regards, Axel
Hi again, Axel Beckert wrote: Additionally it also exits with exit code zero. Both are not what you were declaring as "tolerable behaviour" as you also required "message on stderr plus nonzero exit status" as I do. I though must admit that "Panic()" is indeed widely used all over the screen source code, so it looks like the right function for this purpose. But if I type Ctrl-A:screen ../t.sh 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 inside a Screen session, I do get the proper error message : too many tokens. (with the empty string before the colon) as a status line message. So there it at least aborts properly. I also tried to modify your patch to at least get a non-zero exit code or a visible error message in your case, but failed so far for both. Regards, Axel
Axel Beckert writes ("Re: Bug#1000138: /usr/bin/screen: command arguments beyond 62 silently discarded"):
How alarming.
Indeed so.
That was my test case.
I will also look at this, but probably not until tonight. Will that
be soon enough for you ?
Ian.
Hi Ian,
Ian Jackson wrote:
I've uploaded 4.9.0-1 last night with urgency=low to catch potential
issues to report upstream before 4.9.1 is out. But I'm open to do
another upload, if we find a patch that clearly alarms the user about
this issue.
Just to make it clear, the following is the test case as I understood
and tried it:
$ screen screen ../t.sh {0..999}
[screen is terminating]
$ echo $?
0
$
It should show an error message in addition or instead of "[screen is
terminating]" and exit with a non-zero exit code. I though would
already accept a patch if even one of the conditions is already better
than now.
Working inside a Screen session with "Ctrl-A : screen" is IMHO less
important as the chance for typing more than 62 parameters manually
into a screen session at the colon prompt seems rather seldom. And it
actually also works — at least if the window is wide enough, see my
new upstream bug report at https://savannah.gnu.org/bugs/?61980
Feel free to work in the debian-bug-1000138 branch at
https://salsa.debian.org/debian/screen/-/tree/debian-bug-1000138
It is currently at the master branch plus your original patch with
"crash" replaced by "die". ("panic" instead of "crash" would be fine
as well.)
Regards, Axel
Axel Beckert writes ("Re: Bug#1000138: /usr/bin/screen: command arguments beyond 62 silently discarded"):
Cool, thanks. I agree with everything you said and will look at this
later.
Ian.
Hi. Thanks for your attention to detail and encouragement and
clarity. I got around to looking at this.
tl;dr:
I think in fact the patch is working as designed but your particular
test scenario is equivalent to "screen screen --invalid-argument",
which has a separate (and more fundamental) lost-error bug.
Let me expand on that. This is all quite confusing. I hope you will
spot if I have got something wrong.
You report:
$ screen screen ../t.sh {0..999}
[screen is terminating]
$ echo $?
0
That is indeed the behaviour I see (in all cases, I'm using your
salsa/debian-bug-1000138 branch). I agree that this is not correct
overall behaviour. (But I think the behaviour without my patch is
worse.)
I also observe:
$ screen ../t.sh {0..999}
<screen clears>
1000
<5 second pause>
[screen is terminating]
$ echo $?
0
$ screen
<screen clears, intro message, press Return>
$ screen ../t.sh {0..999}
Argument count or length limit exceeded
$ echo $?
1
$ ^D
[screen is terminating]
$ echo $?
0
I think those are correct. Well, strictly: the first is correct, and
the second is tolerable.
And:
$ screen false
<screen clears, and then instantly:>
[screen is terminating]
$ echo $?
0
I think there is a strong argument that this last one is wrong. But
it's not entirely clear what the semantics ought to be in the general
case. Should a terminating session convey the exit status of the last
window to the outer environment, and thus to all attached clients ?
My patch changes the behaviour of your case
screen screen ../t.sh {0..999}
from
silently forgets all but the first 62 arguments
processes the first 62
discarding any the final error messages the and exit status
to
immediately quit "successfully" having done nothing
I think this is an improvement. Also "screen screen wombat" is a
funny thing to run, since one can start a screen containing only a
session running "wombat" with "screen wombat".
Indeed,
screen screen --this-is-not-a-screen-argument
also just clears the screen and exits claiming success.
What this demonstrates is that "screen anything" has an inherent
tendency to lose errors from "anything".
When "anything" is screen itself, I still think it it is better for it
to do nothing and try to complain, than for it to pass only some of
its arguments and claim that everything is fine. Even if it so
happens that here, the complaints are themselves being lost.
So I think you should probably ship my patch.
Users who are afflicted by a mysterious failure to do anything when
they run
screen screen program thing with many many arguments
will at least know that something funny is going on and if they resort
to
screen
screen program thing with many many arguments
they will see the message. Hopefully will think of doing that, or
simply be discouraged from trusting screen so much - that's not great,
but it's better than gaslighting the user.
Out of interest I did this
$ ttyrec
$ screen screen ../t.sh {0..999}
<screen clears>
[screen is terminating]
$ ^D
$ ipbt ttyrecord
<press "o" then use "space" and "b" to go forwards and backwards">
and you can see that the error message is in there - it just flashes
up too briefly to see. (ipbt is in the package its-playback-time)