Cfrtjryk

This function takes a string from an HTTP POST or GET string, finds the specified variable, and allocates memory for the result and puts it in a string. The destination is given as an address of an empty pointer.

username=johndoe&password=password123

Would produce:

password123

when finding variable password.

void httpString(char **dest, char *input, const char *find) {

char *start;

char *o_input = input;

const char *o_find = find;

size_t length = 0;

size_t i = 0;

while (*input) {

    if (*input == '&' || input == o_input) {

        if (*input == '&') {

            input++;

            if (*input == 0) {

                return;

            }

        }

        while (*input == *find) {

            if (*input == 0 || *find == 0) {

                return;

            }

            input++;

            find++;

            if (*input == '=' && *find == 0) {

                input++;

                if (*input == 0) {

                    return;

                }

                start = input;

                while (*input != '&' && *input) {

                    input++;

                    length++;

                }

                *dest = malloc(length + 1);

                input = start;

                while (*input != '&' && *input) {

                    (*dest)[i] = *input;

                    input++;

                    i++;

                }

                (*dest)[i] = 0;

                return;

            }

        }

    }

    find = o_find;

    input++;

}

}

Any feedback related to how this function can be improved would be greatly appreciated. I am worried about potential edge cases where a memory access violation could occur.

The easiest way to improve this function is to use the C standard library. As it is, it's difficult to read; you can't understand what it does at a glance. And it's unnecessary difficult, because most of the building blocks are already available for free in <string.h>:

• find a string in another string with strstr;


• find a character in a string with strchr;


• copy a string with strcpy or a substring with memcpy
  

Once you have simplified your code, you'll have more brain space to care for not so negligeable things such as testing memory allocations:

void httpString(char **dest, char *input, const char *find) {

    char* found = strstr(input, find);

    if (!found) {

        printf("find not found!");

        return;

    }

    char* assign = found + strlen(find);

    if (*assign != '=') {

        printf("ill-formed!");

        return;

    }

    char* value = assign + 1;

    char* end_value = strchr(value, '&');

    if (!end_value) end_value = strchr(value, 0);

    int length = end_value - value;



    *dest = (char*) malloc(length + 1);

    if (!*dest) {

        printf("Not enough memory");

        return;

    }

    memcpy(*dest, value, length);

    (*dest)[length] = 0;

}

In both GET query strings and POST bodies, the key-value pairs are percent-encoded. Therefore, username and %75%73%65%72name would be considered semantically equivalent keys, and your parser should also look for the percent-encoded variants in the input. Conversely, the function should automatically percent-decode any value that it finds, both for symmetry and utility.

Why not return the result instead of returning void?

However, I'd prefer a design that avoids malloc() altogether, because malloc() could fail, and your caller could easily forget to free() the allocated memory. Consider writing a parser that helps you iterate over the keys and values, overwriting the input with the decoded results. It's kind of ugly, but avoids malloc() altogether.

#include <stdio.h>

#include <stdlib.h>

#include <string.h>



/**

 * Percent-decodes a string in-place.

 */

static void percentDecode(char *s) {

    /* TODO */

}



/**

 * Returns a pointer to the beginning of the a key-value pair, writing

 * a NUL delimiter to the input.  Advances input to the next key-value pair.

 */

char *keyValuePair(char **input) {

    return strsep(input, "&");

}



/**

 * Splits keyValue into two strings, and performs percent-decoding on both.

 * Returns a pointer to the key, and advances keyValue to point to the value.

 */    

char *extractKey(char **keyValue) {

    char *key = strsep(keyValue, "=");

    percentDecode(key);

    percentDecode(*keyValue);

    return key;

}



int main() {

    char *input = strdup("username=johndoe&password=password123");

    for (char *key; (key = keyValuePair(&input)); ) {

        char *value = key;

        if (0 == strcmp("password", extractKey(&value))) {

            printf("Found %s: %sn", key, value);

        }

    }

    free(input);

}

Passwords and library functions

Code dealing with passwords needs to be careful about calling library functions that are not secure as those functions might leave copies of data lingering who-knows-where or leak timing information. That is a good reason to not call standard functions. Still, for developing code, better to first use standard functions and then later replace with secure code.

Flaw: Ambiguous allocation

httpString(char **dest, ) along some paths will allocate memory for *dest, but not all. Function lacks any notification to the caller if allocation occurred or not. This is one of those "I am worried about potential edge cases".

const

As char *input data does not change, add const for greater applicability and potential optimizations.

//void httpString(char **dest, char *input, const char *find) {

//  char *start;

//  char *o_input = input;



void httpString(char **dest, const char *input, const char *find) {

  const char *start;

  const char *o_input = input;

Minor

No allocation check

*dest = malloc(length + 1);

if (*dest == NULL) {

  // do something

Missing proto

Add #include <stdlib.h> for malloc().

Unneeded code

while (*input == *find) {

  // if (*input == 0 || *find == 0) {

  if (*input == 0) {

    return;

  }

Naming

char *input is not that useful. Yes it is input, but input about what?

For such searching goals, code could use boring s1, s2 like C lib strstr(const char *s1, const char *s2), yet I prefer something more illustrative.

void httpString(char **dest, const char *src, const char *pattern)

... or more fun: Needle in a haystack

void httpString(char **dest, const char *haystack, const char *needle)

Candidate alternative:

char *httpString(const char * restrict haystack, const char * restrict needle) {

  size_t needle_len = strlen(needle);

  while (*haystack) {

    if (*haystack++ == '&' && strncmp(haystack, needle, needle_len) == 0

        && haystack[needle_len] == '=') {

      haystack += needle_len + 1;

      size_t password_len = strcspn(haystack, "&");

      char *pw = malloc(password_len + 1u);

      if (pw == NULL) {

        return NULL; // Out of memory

      }

      pw[password_len] = '';

      return memcpy(pw, haystack, password_len);

    }

  }

  return NULL;  // Not found

}