On 19/08/2015 15:29, Edward Bartolo wrote:
This is the completed C backend with all functions tested to work. Any suggestions as to modifications are welcome.
OK, someone has to be the bad guy. Let it be me. First, please note that what I'm saying is not meant to discourage you. I appreciate your enthusiasm and willingness to contribute open source software. What I'm saying is meant to make you realize that writing secure software is difficult, especially in C/Unix, which is full of pitfalls. As long as you're unfamiliar with the C/Unix API and all its standard traps, I would advise you to refrain from writing code that is going to be run as root; if you want to be operational right away and contribute system software right now, it's probably easier to stick to higher-level languages, such as Perl, Python, or whatever the FotM interpreted language is at this time. It won't be as satisfying, and the programs won't be as efficient, but it will be safer. On to the code review.
//using namespace std;
This is technically a nit, but philosophically important: despite of what they've told you, C and C++ are not the same language at all, and when you're writing C, you're not writing C++. So, if you're going to write a C program, forget your C++ habits, don't even put them in comments.
#define opSave 0 (...) #define opLoadExisting 7
As you've been suggested: use an enum.
const char* path_to_interfaces_files = "/etc/network/wifi";
Aesthetics: avoid global variables if you can. If your variable needs to be used in several functions in your module, declare it static: "static const char *path_..." so at least it won't be exported to other modules if you add some one day.
1) Glib::spawn_sync instead of a pipe stream, provides a slot.
I don't understand this comment. It's C++ syntax again, and about the Glib module. Please don't try to explain what you're doing with analogies to other modules in other languages.
2) cmd trying to call an inexistent command still returns a valid pointer! verify cmd exists before calling exec
... and don't do that, because it creates what is known as a TOCTOU race condition: you check that the cmd exists, then you use it, but in the meantime, it could have disappeared. Or the cmd might not exist when you check it, but would have appeared before you used it. So, in essence, the check is useless. What you need is a proper error code when you try to execute a command that does not exist; if necessary, change your API.
inline int file_exists(char* name) { return (access(name, F_OK) != -1); }
And the function isn't actually used in your code either, so you should simply remove its definition.
if(fgets(buffer, buf_size, pipe) != NULL)
If a line is longer than 127 bytes, it will be silently truncated to 127. Is that what you want? If yes, it's fine, but you should document the behaviour.
if (out != NULL) strcat(out, buffer); else strcpy(out, buffer);
This makes no sense. strcpy() can't be called with a NULL argument any more than strcat can. What are you trying to accomplish here?
return pclose(pipe);
This is a bit more advanced. When you design a function, or an interface in general, you want to make it as opaque as possible: you want to report high-level status to the caller, and hide low-level details. Here, the caller does not know you used popen(), and does not want to know. The caller only wants the result in the "out" buffer, and maybe a 0 or 1 return code (possibly with errno set) to know whether things went OK or not. Returning the result of pclose() leaks your internal shenanigans to the caller: don't do that. Instead, design a proper interface (e.g. OK -> nonzero, NOK -> 0 with errno set) and write your function so that it implements exactly that interface.
int saveFile(char* essid, char* pw) //argv[2], argv[3] { char ifilename[1024]; strcpy(ifilename, path_to_interfaces_files); strcat(ifilename, "/"); strcat(ifilename, essid);
Boom. You're dead. I'm stopping here, but you're making this mistake in all the rest of your code, and this is the *single worst mistake* to make as a C programmer: a buffer overflow. A buffer overflow is the main cause of security issues all over the computer world, and you cannot, you ABSOLUTELY CANNOT, publish a piece of code that contains one, especially when said piece of code is supposed to run as root. ifilename is 1024 bytes long. You are assuming that essid, and whatever comes afterwards, will fit into 1024 bytes. This is true for normal inputs, which is certainly what you tested your program against, but the input is given as a command line argument to your program: you do not control the input. *The user* controls the input. And a malicious user could very well give an essid argument that is longer than 1024 bytes. Your program would then either crash (in the best case), or fly demons through your nose, or silently do very evil things that the malicious user intended. Never, ever use fixed-size buffers to handle input you do not have control over. You have two choices here. The simplest choice is to sanitize your input: before calling the functions, you make sure that your arguments are not longer than a given maximum. You adjust the size of your buffer so that the biggest possible data fits. The other choice is to use a variable length buffer: you malloc() a string, then copy your data into it, but if the data is bigger than your allocated pool, you realloc() it, and so on, so you never write data to memory you haven't allocated. It's a bit painful to do that in pure C without any external help, but there are a lot of libraries that provide interfaces to help you do that. My own library has such an interface, "stralloc", and I use it all the time. It's invaluable. So there you go. C/Unix is hard; I love it, but it requires a certain frame of mind, and paying attention to a lot of details. Please fix your buffer overflow NOW, before enthusiasts copy the code and use it as is. Please fix the other, smaller things at your convenience. I'll be available if you have questions. -- Laurent _______________________________________________ Dng mailing list Dng@lists.dyne.org https://mailinglists.dyne.org/cgi-bin/mailman/listinfo/dng