# Tag Info

22

You wrote a nice and simple shell. It works for very simple commands but fails on more complex ones (for details, see below). The code can be cleaned up in several places. To make your code nicely readable by others, let an automatic formatter take care of the indentation and spacing. If you have GNU Indent available, use the following command line: indent ...

18

I see two more security issues with this code. Executing a string rather than an array of command and parameters When building a command line programmatically, it is dangerous to build it as a string, especially when any of the parameters comes from user input. When calling Kernel#system with a string, a shell interprets the command line; when calling it ...

12

Bug This line: argv_copy[argc] = (char *) NULL; should be: argv_copy[argc-1] = NULL; You are removing one argument so you need to terminate the array at the right place. Copy unneeded Instead of: execvp(argv[1], argv_copy); you could do: execvp(argv[1], &argv[1]); and avoid making a copy.

11

There are a lot of functions that aren't safe to call in a signal handler. For the few that are allowed, look at the Async-signal-safe functions section of the signal(7) man page. Your current signal handler is almost okay, but any variable that might be changed from a signal handler needs to be declared volatile. Officially it must also be sig_atomic_t ...

11

lsblk The -s option to lsblk was introduced to util-linux rather recently, in release 2.22. You may experience compatibility issues on slightly older GNU/Linux installations. But I don't see why you would want the -s option at all — it just gives you an inverted device tree. For example, on my machine: $lsblk -o name -n -s -l sda1 sda sda2 sda sr0 vg-... 11 Your defense strategy is known as "enumerating badness". As a rule, any strategy based on enumerating badness is doomed to fail. For example, you prohibit rm -rf /, but have you considered find / -delete, which basically does the same thing? How about some use of the dd if=/dev/zero? What if a user legitimately wants to perform an operation on a file ... 10 It fails silently when you pass it gibberish. Not quite a bug, but still not the best.$ ./a.out nemo 8620 $echo$? 0 $nemo bash: nemo: command not found$ echo $? 127 An easy fix would be to print an error message and return 127 after the execvp. 9 Overall this is some very nicely written C, well done. Some stuff I noted aside from @Morwenn's good points: Portability: <mqueue.h> is a POSIX C library. Unfortunately this restrains the platforms that you compile this for (I couldn't compile this on my Mac without some fiddling around). There are two ways you could fix this: Include the header ... 9 Generally speaking, the code looks good. It seems to be clean and to achieve what it is meant to achieve. Since your Makefile was given, as well as the compiler, I will review your code in function of what is legal with the -std=gnu89 compiler option, which means POSIX C89 + parts of C99 + some other compiler extensions. Here is a list of the allowed ... 9 Looks pretty nice to me. A few remarks: There is no need for the malloc - your structures are small enough so they can live on the stack of main. So you could just do: int main() { char cmd[100]; char* params[10]; .... } then you don't need the free call. It also has the advantage that instead of fgets(cmd, 100, stdin) you can do fgets(cmd, ... 9 First off, a couple of quick tips: Follow the PEP8 style guide Instead of x == False, use not x, for example in if (is_legal_ip(args.ip_port[0]) == False) Omit unnecessary imports (import socket in this example) Use spaces around operators, for example all(0 <= int(p) < 256 for p in pieces) Break lines after colons, for example in if __name__ == "... 9 The obligatory other solution solving your life: def timer(f): def wrapper(job_args, *args, **kwargs): fn_args, timeout, timeout_callback = job_args[:3] q = Queue() p = Process(target=f, args=(q, fn_args), kwargs=kwargs) p.start() p.join(timeout=timeout) p.terminate() p.join() if not q.... 8 You seem to be repeatedly performing a lot of operations on both 'command' and 'public' together. The way these variables are related make me think they should actually be part of the same object. For example: class Command attr_reader :command, :loggable def initialize(command) @command = command @loggable = command end def concat(option) ... 7 It seems to me that this would be equivalent but shorter: def get_sha(): repo = os.listdir('.')[0] sha = subprocess.check_output(['git', 'rev-parse', 'HEAD'], cwd=repo).decode('ascii').strip() return sha Because, . is essentially the same as os.environ['PWD'], with the difference that it's not an absolute path. Since subprocess.check_output(...,... 7 Note that you can also do this trivially with a shell script: #!/bin/sh echo "PID:$$" exec "$@" That's a lot simpler and less bug-prone than doing it in C.

6

execvp may fail on a number of reasons, command not found being just one of them (man execve for the list of possible errors. Check errno and call strerr(errno) for a precise error reason and message. Beware that fork may fail too. It is necessary to test pid against -1, and again check errno and call strerr(errno).

6

Disclaimer: I haven't actually got to trying to run your code yet, this was all done by inspection. However, I think there's enough here to be getting on with... Style Style is important; it makes your code easier to read and understand. Python has a style guide, and (unless you're following a different one*) you should follow it, for example: your imports ...

6

5. All code embedded in a single PHP script. With explanation as requested. The statements before the fork is executed normally. After a succesful pcntl_fork(), the running process is cloned, complete with its current state (all variables, including process counter). So two identical processes continue from that point in code. Almost identical: the only ...

6

In my opinion, Popen is simply the wrong approach to this problem. Everything else follows from that. Python provides an API to directly expose C functions to Python. Boost.Python is a fantastic library to make it easy to expose C++ classes without having to deal with the messy direct Python API. Boost.Numpy makes it easy to return numpy arrays directly. ...

6

In general your script looks good when considering the style and structure, but there are a few issues: Strange name for check_ntp_time – This holds the time since the last time you did time.time(), and in reality is a temporary variable for when you need to check it the last time. It doesn't convey the real purpose of the variable. I would rather do ...

6

Cosmetic issues # include <foo.h> This is ok but unusual, don't put a space between the # and include unless you have a good reason to (sometimes used to show some "indentation" with conditional includes). //Shared Global variables to control processes Those are only used inside main, so no need to make them global. Better would be to ...

6

Memory leaks Since this is part of a shell and can therefore be considered an extension of the OS which should not stop at any point, you have a memory leak problem. Unlike Java and Python there is no garbage collection, you have to manage the memory on your own. Any malloc() calls you make either explicitly or implicitly with functions such as strdup() ...

6

Use of _t You may want to reconsider use of _t endings in your type names. In short, most of the standard type names use this, and the general practice is to not do so with user-defined types. To quote POSIX: To allow implementors to provide their own types, all conforming applications are required to avoid symbols ending in "_t", which permits the ...

5

In your original post, you wrote The variable result is an integer representing the exit code of a command line process that was run via Groovy code. An exit code of 0 means the process was successful and an exit code from 1-255 means a failure. Then this line is executed: println result ? "The command failed." : "The command succeeded." ...

5

You didn't specify that you wanted to leave a tags.txt file as an intentional side-effect of your script. I'm going to assume that it's an unwanted temporary file. In that case, you can read the output of hg directly through a pipe. Furthermore, the file object can be used as an iterator to fetch just two lines, with no for-loop. from subprocess import ...

5

First of all, I don't care for this style of method naming in the slightest. This is Objective-C. Our method names should be verbose and descriptive. If the method name is too long, code completion will help us out. But if it's too short, and we have other methods that perform similar tasks, we may easily get confused. This is particularly true when the ...

5

I am not familiar enough with process control to find any of the problems you seek, so treat this as a general review, for what it is worth, starting from the top: I don't know what advantage your IGNORE_ERRORS macro has over a simple (void) cast for an ignored return value. I imagine you have a good reason for using _exit in child_startup_err rather than ...

5

This kind of parsing by pattern matching is much better done using a regular expression. import re libraries = {} for line in ldd_out.splitlines(): match = re.match(r'\t(.*) => (.*) \(0x', line) if match: libraries[match.group(1)] = match.group(2)

5

This is my biggest complaint about the half-cuddled else: } // Parent process else { One of the problems with C is that the } at the end of a block after an if may mean that you are done with the if or it may mean that there is an else. The C compiler won't care if you have a hundred lines of whitespace before the else, but people have a reasonable ...

5

Security It's not safe to execute commands using system. Try to use one of the execv functions instead (see man execv for the flavors that exist in your system). Although you wrote that only absolute paths can be executed, there's nothing in this code to ensure that. If a malicious user gains access to the rule file, they can enter a relative path, and ...

Only top voted, non community-wiki answers of a minimum length are eligible