Return early when possible
Firstly, well done applying the points from earlier review. One thing you seemed unsure of is early return - I'll restructure your code here to show what that looks like (with extra comments I wouldn't include in real production code):
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main(void)
{
char *stdInData;
char *requestMethod;
char *contentLenString;
int contentLength;
char *user;
char *pass;
char *stdInDataCpy;
requestMethod = getenv("REQUEST_METHOD");
if ( strcmp(requestMethod,"POST") != 0 ) {
printf("Status: 405 Method Not Allowed\n\n");
/* the program successfully detected the user error! */
return EXIT_SUCCESS;
}
/* If we got here, the request method is POST */
contentLenString = getenv("CONTENT_LENGTH");
contentLength = atoi(contentLenString);
if ( contentLength == 0 ) {
printf("Status: 400 Bad Request\n\n");
/* the program successfully detected the user error! */
return EXIT_SUCCESS;
}
/* If we got here, we have a content length */
stdInData = malloc(contentLength + 1);
if (!stdInData) {
printf("Status: 500 Server Error\n\n");
return EXIT_FAILURE;
}
/* If we got here, we have a buffer to read into */
fgets(stdInData, contentLength + 1, stdin);
stdInDataCpy = malloc(contentLength + 1);
if ( stdInDataCpy ){
strcpy(stdInDataCpy, stdInData);
pass = strtok(stdInDataCpy, "&");
pass = strtok(NULL, "&");
pass = strtok(pass, "=");
pass = strtok(NULL, "=");
}
user = strtok(stdInData, "&");
user = strtok(user, "=");
user = strtok(NULL, "=");
printf("Content-type: text/html\n\n");
printf("<html>\n");
printf("<head>\n");
printf("<title>Takelogin</title>\n");
printf("</head>\n");
printf("<body>\n");
printf("%s@%s", user, pass);
free(stdInData);
free(stdInDataCpy);
printf("</body>\n");
printf("</html>\n");
return EXIT_SUCCESS;
}
Do you see how the code is no longer so deeply indented? And we no longer have if within conditional code, so we only need to track one condition at a time?
Add more defensive checks
This first refactoring helps us identify some library calls whose failure we haven't handled well:
getenv() can return a null pointer if the environment variable is unset.fgets() will return a null pointer if there's an error.- If the second
malloc() fails, we ignore it, and pass is uninitialised.
strtok() can return a null pointer.printf() can fail too, in which case we haven't performed our function properly (we can pick up the failure when closing the stream).
The next version, with these checks added (and with variable declaration moved to where they can be initialised):
int main(void)
{
const char *const requestMethod = getenv("REQUEST_METHOD");
if (!requestMethod || strcmp(requestMethod,"POST") != 0) {
printf("Status: 405 Method Not Allowed\n\n");
return EXIT_SUCCESS;
}
const char *const contentLenString = getenv("CONTENT_LENGTH");
int contentLength = contentLenString ? atoi(contentLenString) : 0;
if (contentLength == 0) {
printf("Status: 400 Bad Request\n\n");
return EXIT_SUCCESS;
}
char *const stdInData = malloc(contentLength + 1);
char *const stdInDataCpy = malloc(contentLength + 1);
if (!stdInData || !stdInDataCpy || !fgets(stdInData, contentLength + 1, stdin)) {
printf("Status: 500 Server Error\n\n");
return EXIT_FAILURE;
}
strcpy(stdInDataCpy, stdInData);
const char *pass = strtok(stdInDataCpy, "&");
pass = strtok(NULL, "&");
pass = strtok(NULL, "=");
pass = strtok(NULL, "=");
const char *user = strtok(stdInData, "&");
user = strtok(NULL, "=");
user = strtok(NULL, "=");
if (!pass || !user) {
printf("Status: 400 Missing data\n\n");
free(stdInData);
free(stdInDataCpy);
return EXIT_SUCCESS;
}
printf("Content-type: text/html\n\n"
"<html>\n"
"<head>\n"
"<title>Takelogin</title>\n"
"</head>\n"
"<body>\n"
"%s@%s"
"</body>\n"
"</html>\n",
user, pass);
free(stdInData);
free(stdInDataCpy);
return fclose(stdout) ? EXIT_FAILURE : EXIT_SUCCESS;
}
User input is hostile
Input from outside can be harmful. For example, what happens if we pass a negative content-length parameter?
Even when not actively hostile, well-intentioned user agents can pass the form variables user and pass in any order, and might provide more variables than we expected, so we can't rely on the exact position of our required arguments: we need to inspect them more thoroughly.
Finally, the variables may contain text that we don't want to just include directly into our text (i.e. a cross-site scripting or XSS attack).
We can use a couple of helper functions to make the code much more robust:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* return value (to right of = sign) if s begins with key=, else null */
static const char* val_if_param(const char *s, const char *key)
{
const size_t key_len = strlen(key);
if (!strncmp(s, key, key_len) && s[key_len] == '=') {
return s + key_len + 1;
}
return NULL;
}
static void html_print(const char *s)
{
for (; *s; ++s) {
switch (*s) {
case '&': printf("&"); break;
case '<': printf("<"); break;
default: putchar(*s); break;
}
}
}
int main(void)
{
const char *const requestMethod = getenv("REQUEST_METHOD");
if (!requestMethod || strcmp(requestMethod,"POST") != 0) {
printf("Status: 405 Method Not Allowed\n\n");
return EXIT_SUCCESS;
}
const char *const contentLenString = getenv("CONTENT_LENGTH");
int contentLength = contentLenString ? atoi(contentLenString) : 0;
if (contentLength <= 0) {
printf("Status: 400 Bad Request\n\n");
return EXIT_SUCCESS;
}
++contentLength; /* allow for null terminator */
char *const stdInData = malloc((size_t)contentLength);
if (!stdInData || !fgets(stdInData, contentLength, stdin)) {
printf("Status: 500 Server Error\n\n");
return EXIT_FAILURE;
}
const char *user = NULL;
const char *pass = NULL;
for (char *s = strtok(stdInData, "&"); s; s = strtok(NULL, "&")) {
if (!pass) { pass = val_if_param(s, "pass"); }
if (!user) { user = val_if_param(s, "user"); }
}
if (!pass || !user) {
printf("Status: 400 Missing data\n\n");
free(stdInData);
return EXIT_SUCCESS;
}
puts("Content-type: text/html\n\n"
"<html>\n"
"<head>\n"
"<title>Takelogin</title>\n"
"</head>\n"
"<body>");
html_print(user);
putchar('@');
html_print(pass);
puts("\n</body>\n"
"</html>\n");
free(stdInData);
return fclose(stdout) ? EXIT_FAILURE : EXIT_SUCCESS;
}
