Beefy Boxes and Bandwidth Generously Provided by pair Networks
Welcome to the Monastery

Re: wget not working from perl

by roboticus (Chancellor)
on Sep 10, 2019 at 17:51 UTC ( #11105976=note: print w/replies, xml ) Need Help??

in reply to wget not working from perl


In your first section, you tell the user 'Argument missing.' when they provide too many arguments. That's misleading, I'd change that to something like "Please provide only one URL" or similar.

In your Download() function, I see a couple things:

  • You're constructing a wget command and then running it, but causing yourself a bit of grief here: Using the backticks (or qx) executes the command as a shell command, meaning that you have to take extra care quoting the arguments--otherwise the shell may make a hash of things for you.
    Even worse is that you're opening yourself up to unexpected command injection, possibly giving a malicious user an opportunity to break into your machine. You can avoid both of these problems by using the system() function to execute a command on your system. By giving it the command and arguments as a list, like this:
    my $err_code = system 'wget', '-q', -O', $FILENAME, $URL; if ($err_code) { if ($err_code == 1) { EXIT(-1, "wget: Generic error code") } elsif ($err_code==2) { EXIT(-1, "wget: Parse error (cmdline, .wget +rc, ...)") } elsif ($err_code==3) { EXIT(-1, "wget: File I/O error") } ... etc. ... }
    you can avoid the quoting difficulties and presenting the shell as an attack surface.
  • Why are you creating a directory on your local machine? Just download the file to a temporary location, send it to the console and delete it. There's no reason you need to create directories in various locations. Additionally, since you're not cleaning the directories up when you're done with them, malicious users could start creating hundreds of thousands of directories on your machine, which can cause your machine to get very slow or even run out of disk space.
  • Speaking of downloading to a temporary location, you can use File::Temp to generate temporary file handles for you, so you don't accidentally make a collision. Your RandomString(8) is unlikely to collide, but File::Temp is a much-used (and tested!) module that will avoid running into files that already exist.

The toJStr() function is a bit suspicious, as it looks homegrown--like a "simple" task where you keep adding in exceptions as you find new ones. What happens if you get a binary string containing UTF-8 characters not in the ASCII range? Should your string be encoded?

One interesting thing might be what happens when you pass it something like "\00765". I didn't test it, but I'm thinking it's not going to end well.

The escape() and unescape() functions look unnecessarily opaque, and I worry that they may be homegrown things that don't have all the special cases handled. If they're "tried and true", you may want to mention that in a comment, as well as where they came from so you can treat them like "black boxes". That way, if some error occurs in the future, you can decide to dig into them or not based on your comment(s).

Also, why do you hate whitespace? If they were formatted better, at least you could look them over and get a feel for what they do.

Your Ceil() and Floor() functions are straightforward enough, that being one-liners seems OK. Even so, I'd still put a little whitespace in there for readability.


When your only tool is a hammer, all problems look like your thumb.

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://11105976]
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (3)
As of 2020-06-05 03:31 GMT
Find Nodes?
    Voting Booth?
    Do you really want to know if there is extraterrestrial life?

    Results (35 votes). Check out past polls.