Re: PHP shell 2.4.1

[Jan K. <jan...@ja...>]

Hi Harding,

Great to see what you did and that the subprocess approach can
actually be used on windows as well. Too bad of all the necessary
encoding stuff.

I had a look through most of the code (though I didn't test it). Here
are a number of things I noticed.

First of all regarding the encoding conversions. In the code, all the
additional encoding conversions are making the code more hairy. More
code goes with having more features, but I have the feeling that all
the encoding stuff could be done in less lines.

You have a lot of instances of code like

$foo = getfoo();
if ($server_is_windows)
    $foo = mb_convert_encoding($foo,
$_SESSION['env']['windows-encoding'], "UTF-8");

I think some lines of code (and complexity) can be saved by defining a
function tonative and the analogous fromnative like so:

function tonative($str) {
    global $server_is_windows;
    if($server_is_windows)
        return mb_convert_encoding($str,
$_SESSION['env']['windows-encoding'], "UTF-8");
    return $str;
}

then every instance of the previous pattern can be replaced with just

$foo = tonative(getfoo());

Reading a bit about windows encoding stuff, I discovered that it may
be possible to remove all the CP850 handling altogether. The command
"chcp 65001" switches the cmd.exe interpreter to use utf-8 instead of
cp850. So would it be possible to execute "chcp 65001 > nul" as the
first command every time a subprocess is invoked, and not needing that
part of the conversion anymore? (Encoding will still be needed for
calls to php filesystem functions).


Some other things I noticed:

- What is 'OBS!' that you use several times in comments?

- On phpshell.php:135 you try to work around a possible deadlock bug
by using different code to read the file descriptors on windows.
However, I'm quite sure that this code can deadlock as well (assuming
non buggy php behaviour). Try executing the foillowing php program as
a subprocess:

$data = str_repeat("a", 10000);
fwrite(STDOUT, "Starting subprocess");
fwrite(STDERR, $data);
fwrite(STDOUT, "Finished");
exit(0);

I think this will deadlock trying to write all the data to stderr,
while phpshell is stuck in an infinite loop on line 138 trying to read
more data from the subprocesses stdout. The regular solution for that
would be something like the *nix code, but I don't know when the 51800
bug will bite or not.


- The encoding detection code is probably better refactored into it's
own function, so the exec_command code flow doesn't get unnecessary
complicated. (If it is needed at all)

- You don't always htmlescape stuff you output, e.g. at line
phpshell:1304. If you use htmlescape you also don't need the
str_replace('"', '"', $dir) anymore.

- Good to have secure randomness on Windows. But I think your
implementation has a problem: you hash the returned random string, but
after that hashing it always has the same length instead of the
requested length.

- I think the unregister_GLOBALS function should be moved with the
other function definitions, instead of defining it in the middle of a
block of imperative code.

- regarding the comment on line phpshell.php:991: The reason to do
things through subprocesses instead of using the php functions is to
work around possible OPEN_BASEDIR restrictions. So using the old code
for windows is fine.

- It's probably more readable to replace the dozen print statements on
line phpshell.php:1346 by going to html mode.

- I noticed you created a substr_unicode function, however it is not
used anywhere.

- Just a note: the code in your zipfile appears to have old versions
of some of the documentation files.

Anyway, good job so far.


Jan


On Tue, Aug 21, 2012 at 7:26 PM, Harding <no...@so...> wrote:
> Jan wrote: I\'m also interested in how you got windows
> compatibility working. Are
> you sure your changes are based on the latest svn version
> and not on
> the released version? There are quite a few changes in svn
> which I
> thought could not be made windows compatible, but maybe I\'m
> wrong. So is your code somewhere available online?
>
> Harding: Yes I checked out the new code from the SVN and
> continued from that base. I had to solve some parts \"ugly\"
> for the windows version due to some bugs in PHP for windows
> :-( (https://bugs.php.net/bug.php?id=51800) But most of the
> code is the same with some encoding magic due to Windows
> specific encoding depending on your location in the world. I
> have made quite some changes to the core code, UI and the
> config and I hope you got time to read it.
>
> You can find the code at
> http://hardingonline.se/phpshell_2.4.1.zip
>
> I would love to hear what you think about it :-)
>
> --
> This message was sent to your SourceForge.net email alias via the web mail form.  You may reply to this message via https://sourceforge.net/sendmessage.php?touser=3934724
> To update your email alias preferences, please visit https://sourceforge.net/account