Bug 486844

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.

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

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

Great, thank you for review the patch and finding problems. Don't you want become a co-maintainer? ;-)

(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 ;)