bugGNU Screen - Bugs: bug #25813, [REGRESSION] Not working: screen...

 
 

bug #25813: [REGRESSION] Not working: screen -X stuff "a b"

Submitted by:  Micah Cowan <micahcowan>
Submitted on:  Tue 10 Mar 2009 05:41:22 PM UTC  
Votes:  15  
 
Category: Program LogicSeverity: 4 - Important
Priority: 5 - NormalStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: ClosedRelease: None
Fixed Release: 4.1.0Planned Release: 4.1.0
Work Required: 0 - Hours

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 26 Mar 2010 11:47:24 PM UTC, comment #12:

Thanks Max for pointing out the problem. This should properly be fixed with ad8910f2. http://git.savannah.gnu.org/cgit/screen.git/commit/?id=ad8910f21f7df0972aa8bbdafcd60686bd931eca

Sadrul Habib Chowdhury <sadrul>
Project Administrator
Mon 22 Mar 2010 03:32:28 PM UTC, comment #11:

Indeed. We might need to resort to escaping after all. Need to take a closer look. Thanks for the report.

Sadrul Habib Chowdhury <sadrul>
Project Administrator
Mon 22 Mar 2010 05:00:12 AM UTC, comment #10:

The simpler fix is only for the titular example, when there is whitespace.

There is still a regression with trying to stuff a double quote.

Max Kalashnikov <maxkalashnikov>
Sun 21 Mar 2010 12:53:50 AM UTC, comment #9:

Thanks for the patch, Max.

But I thought there can be a simpler fix for this. I have committed the fix in f7adfae.

http://git.savannah.gnu.org/cgit/screen.git/commit/?id=f7adfae856bb83107024559bf3b167bc4b9a42df

Sadrul Habib Chowdhury <sadrul>
Project Administrator
Fri 28 Aug 2009 06:50:52 AM UTC, comment #8:

I've attached a patch that should do the trick.

Essentially, it double quotes all -X arguments and escapes any already-present double quotes, before sending them.

(file #18652)

Max Kalashnikov <maxkalashnikov>
Fri 28 Aug 2009 04:09:27 AM UTC, comment #7:

I see what you mean, and, of course (as a sysadmin by trade), I agree.

Sadly, one can't have both full parsing (which strips quotes) and preservation of quotes.

However, since you don't necessarily want full parsing, perhaps a modified Parse() would do the trick, as might a quote-escaper on the SendCmdMessage() frontend. I'll see which, if either, is easy.

Max Kalashnikov <maxkalashnikov>
Thu 27 Aug 2009 05:54:51 AM UTC, comment #6:

Thanks very much for your analysis, Max.

Actually, I think we knew much of this already (but it's good to have the details all here for reference). I think Sadrul and I have both considered this a regression in behavior, especially since it has the potential to break pre-existing scripts.

I think the ideal solution would be to fix this behavior so it matches previous behavior, but still let ^a et friends have the same special meaning they do in an rc file. That is, parsing should be as it is for the rc file (which is what the #24924 fix did), except that it should pretend (where necessary) that we're already in the midst of a string literal (not require extra quotes).

This might be more work than it's worth; I think Sadrul indicated he was considering pulling his #24924 fix. I'd be bummed in this case, but I agree it's better than breaking pre-existing behavior. One can always insert a literal ^a if necessary: the shell always provides tricks for this; but breaking historical syntax shouldn't happen.

Micah Cowan <micahcowan>
Thu 27 Aug 2009 05:30:15 AM UTC, comment #5:

98b6b410 fixing #24924 did this, so this is, perhaps, not a regression, in that the code now correctly parses commands sent in via -X.

This should probably be just a documentation change, making it clear that whatever is passed to -X is subsequently parsed by the backend screen, as in .screenrc, as a string, without regard to original argument tokenizing.

Max Kalashnikov <maxkalashnikov>
Thu 27 Aug 2009 04:54:15 AM UTC, comment #4:

Oops.. sorry

Max Kalashnikov <maxkalashnikov>
Thu 27 Aug 2009 04:44:56 AM UTC, comment #3:

It's definitely in this change:

<pre>
@@ -1339,8 +1565,10 @@ { char *args[MAXARGS];
int argl[MAXARGS];
- int n, *lp;
- register char *pp = args, p = mp->m.command.cmd;
+ char fullcmd[MAXSTR];+ register char *fc;
+ int n;
+ register char *p = mp->m.command.cmd;
struct acluser *user;
#ifdef MULTIUSER extern struct acluser EffectiveAclUser; / acls.c /@@ -1348,17 +1576,21 @@ extern struct acluser users; /* acls.c */
#endif

- lp = argl;
n = mp->m.command.nargs;
if (n > MAXARGS - 1)
n = MAXARGS - 1;
- for (; n > 0; n--)
+ for (fc = fullcmd; n > 0; n--)
{
- *pp++ = p;
- *lp = strlen(p);
- p += *lp++ + 1;
- }
- *pp = 0;
+ int len = strlen(p);
+ strncpy(fc, p, fullcmd + sizeof(fullcmd) - fc - 1);
+ p += len + 1;
+ fc += len;
+ *fc++ = ' ';
+ }
+ if (fc != fullcmd)
+ *--fc = 0;
+ if (Parse(fullcmd, fc - fullcmd, args, argl) <= 0)
+ return;
</pre>

Max Kalashnikov <maxkalashnikov>
Thu 27 Aug 2009 04:11:56 AM UTC, comment #2:

I also tweaked the MSG_VERSION in the current git sources, and the behavior with a 4.0.3 receiver is unchanged.

Max Kalashnikov <maxkalashnikov>
Thu 27 Aug 2009 04:03:36 AM UTC, comment #1:

I found it instructive to try running this from the colon, as this is somewhere the behavior does not differ, despite the fact that screen itself will strip quotes.

Max Kalashnikov <maxkalashnikov>
Tue 10 Mar 2009 05:41:22 PM UTC, original submission:

In current git sources,
screen -X stuff "a b"
is rejected with "invalid option 'a'", whereas
screen -X "stuff 'a b'"
or
screen -X stuff "'a b'"
will stuff
a b

In 4.0.3 (at least on Ubuntu), however,
screen -X "stuff 'a b'"
fails with "Unknown command: 'stuff 'a b''",
and
screen -X stuff "'a b'"
will stuff
'a b'
(with the quotes)

In Ubuntu 4.0.3 (should check a canonical 4.0.3),
screen -X stuff "a b" c
fails with "invalid option 'a b'", which suggests that the change isn't in number-of-args supported, but just in how shell args get processed by screen.

Micah Cowan <micahcowan>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #18652:  screen-attacher.diff added by maxkalashnikov (1KiB - application/octet-stream - proposed patch, without backing out parsing change)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by sadrul (Posted a comment)
  • -unavailable- added by maxkalashnikov (Voted in favor of this item)
  • -unavailable- added by maxkalashnikov (Posted a comment)
  • -unavailable- added by micahcowan (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 15 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 26 Mar 2010 11:47:24 PM UTCsadrulStatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
      Fixed ReleaseNone=>4.1.0
    Mon 22 Mar 2010 03:32:28 PM UTCsadrulStatusFixed=>Confirmed
      Open/ClosedClosed=>Open
      Fixed Release4.1.0=>None
    Sun 21 Mar 2010 12:53:50 AM UTCsadrulStatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
      Fixed ReleaseNone=>4.1.0
    Mon 07 Sep 2009 11:14:10 PM UTCmaxkalashnikovCarbon-Copy-=>Added maxkalashnikov
    Fri 28 Aug 2009 06:50:52 AM UTCmaxkalashnikovAttached File-=>Added screen-attacher.diff, #18652

    Back to the top


    Powered by Savane 3.1-cleanup1