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