On Wed, Aug 29, 2012 at 9:57 AM, Jimmy Hermansson
<ji...@hardingonline.se> wrote:
> Hi Jan!
>
> Glad you liked it and had the time to read the code. Here are some comments
> on your comments :-)
>
>
> Jan wrote:
> ...
> Jan wrote:
>
> "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)."
>
> This is not entirely true, actually, if you type that and try to list
> (commando dir) folders with strange UTF-8 characters in them the dir
> commando stops and fails.
> I will try to get around the CP850 but I did put a lot of time into
> researching how Windows cmd handles encodings.

Hmm, I did some testing today on a virtual winXP+apache+php. It sucks.
After attempting to change to cp 65001 any further commands are not
executed at all. Single byte codepages work fine. If you think the
current solution works best then so be it. There may be some other
possibilities with cmd.exe's /U switch, but that probably won't be
easy either (if it works at all).

> Jan wrote:
> "What is 'OBS!' that you use several times in comments?"
>
> Leftover notes I wrote while coding/researching. Most of them can be removed
> but I think I used some to explain why I wrote the code as I did. For
> example the
> bug under windows with the deadlock and such.
>
>
> Jan wrote:
> "- 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:"
>
> Yes I know this CAN deadlock but since the other code ALWAYS deadlocks this
> was an alternative until the fixed PHP. I tried to read one line each from
> the pipes but that also deadlocked. This was the only thing I could get some
> results from.

Ah, then I misread your comment of "However, the old code seem to work
good (in all cases I've tried) but I know it _MIGHT_ casuse
deadlock!". I tried it, and I can confirm that your code works better
than the *nix code for windows.

> Jan wrote:
> ....
> Jan wrote:
> "- It's probably more readable to replace the dozen print statements on
> line phpshell.php:1346 by going to html mode."
>
> Here is where I don't agree. The switching back and forth is a horror in my
> opinion. with this I can easy comment out stuff and use correct indent.

We'll agree to disagree then. And it's not like the current code is
formatted very strictly.

> Jan wrote:
> "- I noticed you created a substr_unicode function, however it is not
> used anywhere."
>
> It was used before and I left it as a good utility for later. I guess it can
> be removed.
>
>
> Jan wrote:
> "- Just a note: the code in your zipfile appears to have old versions
> of some of the documentation files."
>
> The zip file was just a way to send you the code. The other files are not up
> to date no. :-)
>
> / Jimmy


And I think you have svn access now right? Then I won't do any commits
for you. If you don't want to commit to the main repo yet, you could
also consider creating a branch while you're still working on it. That
way it would be easier for me and others to see your code, and make
fixes to it.

Also, I prefer having these kinds of mails on the mailing list so
others can find why certain choices were made.


keep up the good work,
Jan


> -----Original Message----- From: Jan Kanis
> Sent: Friday, August 24, 2012 9:14 AM
> To: ji...@hardingonline.se ; phpshell-devel
> Subject: Re: PHP shell 2.4.1
>
>
> 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('"', '&quot;', $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 <nore...@sourceforge.net> 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
>
>

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
phpshell-devel mailing list
phpshell-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/phpshell-devel

Reply via email to