Bug 486844 - Problem with command substitution and here-document in shell scripts generated by "at"
Problem with command substitution and here-document in shell scripts generate...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: at (Show other bugs)
10
All Linux
low Severity medium
: ---
: ---
Assigned To: Marcela Mašláňová
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-22 08:58 EST by Cong Ma
Modified: 2009-03-25 04:32 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-26 08:51:44 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Proof-of-concept test case to show why it doesn't work (565 bytes, text/plain)
2009-02-22 08:58 EST, Cong Ma
no flags Details

  None (edit)
Description Cong Ma 2009-02-22 08:58:59 EST
Created attachment 332854 [details]
Proof-of-concept test case to show why it doesn't work

Description of problem:
Shell scripts generated by "at" as scheduled jobs, I believe, have something wrong with the usage of here-documents and command substitution.

The problem is introduced in this patch: < http://cvs.fedoraproject.org/viewvc/rpms/at/F-10/at-3.1.10-shell.patch?revision=1.1&view=markup > (in function writefile(), file at.c)

I think the author of the patch meant to generate a random "cookie" as end-of-file delimiter of the here-document passed to whatever ${SHELL:-/bin/sh} expands to. This is supposed to help non-BASH users so that they can use their shell of choice, if I read it correctly. However, according to the BASH manual, the stuff inside the backticks are *NOT* expanded in the usual way of command substitution when it is placed after the "<<" operator. (See BASH manual, subsection "Here Documents", in the section titled "REDIRECTION"). As a result, no actual "cookie" is generated.

I have constructed a proof-of-concept test case in the attachment to show how this may potentially mislead the user. Suppose the user's shell is CSH. He invokes "at" and types some CSH commands at the "at> " prompt. Everything would work as intended if he's not too creative. However, if for some reason he types in a new line the following string literally:

`(dd if=/dev/urandom count=200 bs=1 2>/dev/null|LC_ALL=C tr -d -c [:alnum:])`

Then this string will prematurely end the here-document. Whatever he types after this line will not be interpreted by CSH but by sh.

I don't think this is the intended behavior.


Version-Release number of selected component (if applicable):
at-3.1.10-26.fc10


How reproducible:
If the user's shell is not /bin/sh.


Steps to Reproduce:
See above.
1. User choose non-sh shell.
2. Invoke at, follow the procedures discussed above.

  
Actual results:
Anything after the "noisy" line are not interpreted by the user's shell of choice.


Expected results:
Everything should be interpreted by the user's shell of choice.


Additional info:
1. This shouldn't be a problem for daily usage, but anyway, it's something pretty annoying IMO.
2. The fix should be easy. Don't delay the generation of the random "cookie" to job run. Write some suitable random string generator in the C code of at and make the cookie when generating the job script.

Thank you for your time :)
Comment 1 Marcela Mašláňová 2009-02-24 08:21:35 EST
This is probably the most thorough analysis which I'd seen :)

I'm not sure why here was used so creative bash delimiter. This must be fixed anyway because new bash broke this in rawhide #487056.
Comment 2 Marcela Mašláňová 2009-02-26 08:51:44 EST
I hope it will be okay, if I'll fix it only in rawhide. If it's bother you too much then let me know. The random number is now generated by C code as you suggested. 

Fixed in at-3.1.10-30.fc11
Comment 3 Cong Ma 2009-03-16 23:55:28 EDT
Sorry for not replying for such a long time..


I'm not on rawhide so I can't really test the patch. I use the stable F10 exclusively on my only computer so I don't wish it broken :P


Anyway, I took a look at the patch (http://cvs.fedoraproject.org/viewvc/devel/at/at-3.1.10-different_shell.patch?revision=1.1&view=markup), and there seems to be a problem again.. It appears to me that the new "random cookie" code looks like an NOP. I mean these lines in the patch:

-    fprintf(fp, "${SHELL:-/bin/sh} << `(dd if=/dev/urandom count=200 bs=1 2>/dev/null|LC_ALL=C tr -d -c '[:alnum:]')`\n\n");
+    i = random();
+    fprintf(fp, "${SHELL:-/bin/sh} << marcinDELIMITER%x\nmarcinDELIMITER%x\n", i,i);

The relevant part of the shell script generated by this new code would look like this:

...
some_shell << marcinDELIMITERcafe1234
marcinDELIMITERcafe1234
[user-supplied commands]
...


So the script is self-defeating: the user-supplied commands should have been enclosed in the here-document so that they are passed to the "some_shell" which is the expansion result of ${SHELL:-/bin/sh} i.e. the current $SHELL (dumped into the at-generated script earlier) with /bin/sh as last resort.


There's also a minor problem with the variable "i" declared as "int" but should be "long" according to the API. I know this is not a problem on x86/x64 systems since "long" and "int" are the same thing. I point this out just for theoretical correctness ;)


In summary I think the correct code should be something like this:

long int i;
...
i = ramdom();
fprintf(fp, "${SHELL:-/bin/sh} << marcinDELIMITER%lx\n", i);
[... code responsible for putting user input into the file ...]
[just after writing user-provided commands into the file]
fprintf(fp, "marcinDELIMITER%lx\n", i);


Well, I just reviewed the code by my non-expert eyes rather than trying it out, so maybe I missed something.


There could be another minor improvement: making the random number in the cookie looks better by padding zeros i.e. using the format "%08lx" rather than a bare "%lx". However, the standard of "better looking" varies ;)
Comment 4 Marcela Mašláňová 2009-03-18 09:41:09 EDT
Great, thank you for review the patch and finding problems. Don't you want become a co-maintainer? ;-)
Comment 5 Cong Ma 2009-03-25 04:32:22 EDT
(In reply to comment #4)
> Great, thank you for review the patch and finding problems. Don't you want
> become a co-maintainer? ;-)  

Hi,

I just stumbled upon this problem by chance.. Actually I don't I have the skills or time to maintain it. So, the answer is "not now", maybe later..

Anyway, thanks for asking ;)

Note You need to log in before you can comment on or make changes to this bug.