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

 
 

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

Submitter:  Micah Cowan <micahcowan>
Submitted:  Tue 10 Mar 2009 05:41:22 PM UTC
Votes: 15
 
Category:  Program Logic Severity:  4 - Important
Priority:  * 5 - Normal Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Release:  None
Fixed Release:  4.1.0 Planned Release:  4.1.0
Work Required:  0 - Hours
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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>
Group 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

@@ -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;


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 Files:
   
   
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
  • -email is unavailable- added by sadrul (Posted a comment)
  • -email is unavailable- added by maxkalashnikov (Voted in favor of this item)
  • -email is unavailable- added by maxkalashnikov (Posted a comment)
  • -email is unavailable- added by micahcowan (Submitted the item)
  •  

    There are 15 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-03-26 sadrul StatusConfirmed Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.1.0
    2010-03-22 sadrul StatusFixed Confirmed
        Open/ClosedClosed Open
        Fixed Release4.1.0 None
    2010-03-21 sadrul StatusConfirmed Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.1.0
    2009-09-07 maxkalashnikov Carbon-Copy- Added maxkalashnikov
    2009-08-28 maxkalashnikov Attached File- Added screen-attacher.diff, #18652

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code