C lang CGI form/string handle

Question

Started playing with C and CGI forms. Is this code ok? Can it be improved/optimized?

If I fill user with "user" and password with "pass" input in takelogin is user=user&password=pass

In the end I want to have user = 'user' and pass = 'pass'

This is the form

#include <stdio.h>
int main(void)
{
 printf("%s%c%c\n", "Content-Type:text/html;charset=iso-8859-1",13,10);
 printf("<title>Takelogin</title>\n");
 printf("<h3>Multiplication results</h3>\n");
 printf("%s\n", "<form action='/takelogin.cgi' method='post'>");
 printf("%s\n", "<div>Your input (80 chars max.):<BR>");
 printf("%s\n", "<input type='text' name='user' size='60' maxlength='80'><br>");
 printf("%s\n", "<input type='password' name='password' size='60' maxlength='80'><br>");
 printf("%s\n", "<input type='submit' value='Send'></div>");
 printf("%s\n", "</form>");
 return 0;
}

And this is takelogin

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAXINPUT 80
int main(void)
{
 char *lenstr;
 char *user;
 char *pass;
 char input[MAXINPUT];
 long len;
 char delim[2][2] = {"&", '='};
 printf("%s%c%c\n",
 "Content-Type:text/html;charset=iso-8859-1",13,10);
 printf("<TITLE>Response</TITLE>\n");
 lenstr = getenv("CONTENT_LENGTH");
 if(lenstr == NULL)
 printf("<P>Error in invocation - wrong FORM probably.");
 else {
 fgets (input , sizeof(input) , stdin);
 char *loginS = malloc(sizeof(input));
 strcpy(loginS, input);
 user = strtok(loginS, delim[0]);
 user = strtok(user, delim[1]);
 user = strtok(NULL, delim[1]);
 pass = strtok(input, delim[0]);
 pass = strtok(NULL, delim[0]);
 pass = strtok(pass, delim[1]);
 pass = strtok(NULL, delim[1]);
 printf("%s@%s", user, pass);
 free(loginS);
 return 0;
 }
 return 0;
}

• Sᴀᴍ Onᴇᴌᴀ: Welcome to Code Review! I have rolled back Rev 2 → 1. Please see What to do when someone answers.

Answer 1

Some bugs:

• malloc() can fail. When it does, it returns a null pointer, which we must not use as destination for strcpy().
• The fixed-length input array can be overrun by the remote user. That's a pattern that causes security problems in server code (which this is, via CGI). Our strategy to avoid that (by reading only sizeof input - 1 chars) means that we silently change the user's input.

Other issues:

• Instead of reading into input and then copying into loginS, we should allocate memory, then (if successful) call fgets(loginS, size, stdin).

• We should be testing for valid input before we output any headers, as we should be returning a HTTP error code (probably 400 Bad Request) rather than success (200).

• We could parse lenstr, and if successful, use that value to allocate a suitable size buffer instead of sizeof input chars (which will always be MAXINPUT). Don't forget + 1 for a terminating null char!

• I don't think the delim array is helpful - it would be clearer to put the literal strings in the code there. I know that breaks the rule of "no magic values", but I think that here the rule just obfuscates rather than clarifies the code.

• If the printf() fails, it might be a good idea to return EXIT_FAILURE, so the CGI engine knows that something has gone wrong (the user hasn't got their result).

• Instead of if/else, I prefer an early-return pattern:

   if (error_condition) {
   report_error();
   cleanup_resources();
   return EXIT_FAILURE;
   }
  

  That means less context to hold in mind whilst reading.

• Martzy: Have posted an update here, If you want to have a look, thank you for these points. codereview.stackexchange.com/questions/276049/…