http://qs1969.pair.com?node_id=406792


in reply to mksc.pl

Looks cool! A few notes:
  1. Most Perl programs that are meant to be run from the shell use Getopt::Long and Pod::Usage for interfacing with the user. You might want to look at those as they can really simplify your life. Plus, they allow you to easily handle options that are, well, optional. :-)
  2. You use a bunch of tempfiles. The way you're using it means that you cannot safely have two versions of this program running, plus it's not OS-portable the way you've written it. (In fact, it's barely Unix-portable.) I'd recommend using File::Temp which handles all that crap for you.
  3. Although you don't actually use directories and pathnames, I'd also look at File::Spec (and File::Spec::Functions if you don't like the OO interface) for that need.
  4. You use 'rm -rf' instead of the built-in unlink. Why? (Plus, if you use File::Temp, it's taken care of for you.)
  5. It would probably be better to write
    for($i=$addr;$i<$saddr;$i++) { print TEMP "\n" }
    as
    print TEMP $/ for $addr .. $saddr - 1;
    Not only is it more Perlish, but it better documents what it is you're attempting to do.
  6. You don't check the return value for any of your system calls. That's not good ...
  7. Are you charged for your whitespace?
    $x=~s/...//g;
    is hard to read. Why not do something like
    $x =~ s/...//g;
    instead? The compiler doesn't care, but I will. Reading code is hard - you probably want to make it as easy as possible. (Plus, see below for more help in readability.)
  8. Your main loop
    open(ITEMP,"/tmp/scresult.tmp"); @data=<ITEMP>; close ITEMP; foreach $x(@data) { if($x=~/^\(gdb\)/) { $x=~s/\(gdb\) 0x.+ <.+>:\s+//g; $x=~s/0x/\\x/g; $x=~s/\n//g; $x=~s/\(gdb\)//g; $x=~s/\s+//g; $sc.=$x; } $i++; # # "Indent" the shellcode # ($i exists only for this) # if(($i%12)==0) { $sc.="\"\n\t\"" } }
    is probably better written as:
    open( TEMP, "</tmp/scresult.tmp" ) or die "Cannot open: $!\"; my $sc="char ".$func."[]=\n\t\""; my $i = -1; while (<TEMP>) { if(/^\(gdb\)/) { s/\(gdb\) 0x.+ <.+>:\s+//g; s/0x/\\x/g; s/\n//g; s/\(gdb\)//g; s/\s+//g; $sc .= $_; } # "Indent" the shellcode # ($i exists only for this) $i++; unless (++$i%12) { $sc.="\"\n\t\"" } } close TEMP;

    Not only is it clearer what you're doing, but you don't slurp the whole file into memory. What if you're going for 1G of data? Also, you should indent your comments to line up with your code. Otherwise, the whole point of indenting - to show logical groupings - is ruined. When I skimmed your original code, I didn't immediately see the fact that the modulo-check on $i had something to do with the loop.

    Plus, you shouldn't declare a variable until you intend to use it. You declare $sc way up at the top, but don't use it until nearly the end. If you declare it at the top, I'm going to look for usage near the top. If I don't see it, I'm going to start looking for bugs that may not be there.

Being right, does not endow the right to be rude; politeness costs nothing.
Being unknowing, is not the same as being stupid.
Expressing a contrary opinion, whether to the individual or the group, is more often a sign of deeper thought than of cantankerous belligerence.
Do not mistake your goals as the only goals; your opinion as the only opinion; your confidence as correctness. Saying you know better is not the same as explaining you know better.