Capture and assign portions of char array to a struct

Question

I am formally a PHP/C# developer and I am new to C. I need you to help to correct my coding with C.

I have a kind of following pieces of HTTP user data string.

char *userData = "=GET /ad26908aa2e811e3855e0a22f2d2d906_8.jpg HTTP/1.1..Host: distilleryimage10.ak.instagram.com..Connection: keep-alive..Accept-Encoding: gzip, deflate..User-Agent: Instagram 4.2.7 (iPhone6,2; iPhone OS 7_0_6; en_US; en) AppleWebKit/420+..Accept-Language: en;q=1, fr;q=0.9, de;q=0.8, ja;q=0.7, nl;q=0.6, it;q=0.5..Accept: */*....";

I need to capture/slice Host, Query String and httpmethod from the above char string.

My function is below.

int diggForUrl(char *userData, HttpData *httpData)
{
    const char str_GET[]       = "GET";
    const char str_POST[]      = "POST";
    const char str_DELETE[]    = "DELETE";
    const char str_PUT[]       = "PUT";

    const char str_HTTP[]      = "HTTP";
    const char str_HOST[]       = "Host:";

    char httpMethod[10] = {'\0'};
    char *qString = (void *) 0;
    char *host = (void *) 0;

    char *ptr_SRV_CMD = (void *) 0;
    if (ptr_SRV_CMD = strstr (userData, str_GET)) {
        strncpy(httpMethod, str_GET, strlen(str_GET));
    }
    else if (ptr_SRV_CMD = strstr (userData, str_POST)) {
        strncpy(httpMethod, str_POST, strlen(str_POST));
    }
    else if (ptr_SRV_CMD = strstr (userData, str_DELETE)) {
        strncpy(httpMethod, str_DELETE, strlen(str_DELETE));
    }
    else if (ptr_SRV_CMD = strstr (userData, str_PUT)) {
        strncpy(httpMethod, str_PUT, strlen(str_PUT));
    }

    if (ptr_SRV_CMD == NULL)
        return -1;

    char *ptr_HTTP = strstr(userData, str_HTTP);
    if (ptr_HTTP && ptr_SRV_CMD) {
        char *ptr_qs_start = ptr_SRV_CMD + strlen(httpMethod) + 1;
        // Compare the HTTP position and METHOD position
        if (ptr_HTTP > ptr_qs_start) {
            // Create query string size
            int qsSize = ptr_HTTP - ptr_qs_start;
            qString = malloc(qsSize + 1);
            memset(qString, '\0', (qsSize + 1));
            strncpy(qString, ptr_qs_start, qsSize);
        }
    }

    char *ptr_HOST = strstr(userData, str_HOST);
    if (ptr_HOST){
        char *ptr_HOST_END = strstr(ptr_HOST, "..");
        if (ptr_HOST_END) {
            int hostSize = ptr_HOST_END - (ptr_HOST + (strlen(str_HOST) + 1));
            host = malloc(hostSize + 1);
            memset(host, '\0', hostSize + 1);
            strncpy(host, (ptr_HOST + strlen(str_HOST) + 1), hostSize);
        }

    }

    // copy final value of httpMethod char array to char string
    httpData->httpMethod = malloc(sizeof(httpMethod) + 1);
    memset(httpData->httpMethod, '\0', sizeof(httpMethod) + 1);
    strncpy(httpData->httpMethod, httpMethod, sizeof(httpMethod) + 1);

    httpData->host = host;
    httpData->qString = qString;

    return 0;
}

and struct which I am capturing the data into is

typedef struct {
    char *httpMethod;
    char *host;
    char *qString;
} HttpData;

This code runs and give me expected result. Still I feel the coding is not neat and it can be improved. Also, if I run this with valgrind I am getting the message that there are leaks.

==16470== Memcheck, a memory error detector
==16470== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==16470== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==16470== Command: ./http_data_digg
==16470==
GET
distilleryimage10.ak.instagram.com
/ad26908aa2e811e3855e0a22f2d2d906_8.jpg
==16470==
==16470== HEAP SUMMARY:
==16470==     in use at exit: 87 bytes in 3 blocks
==16470==   total heap usage: 3 allocs, 0 frees, 87 bytes allocated
==16470==
==16470== LEAK SUMMARY:
==16470==    definitely lost: 87 bytes in 3 blocks  // <<======= LOST HERE
==16470==    indirectly lost: 0 bytes in 0 blocks
==16470==      possibly lost: 0 bytes in 0 blocks
==16470==    still reachable: 0 bytes in 0 blocks
==16470==         suppressed: 0 bytes in 0 blocks
==16470== Rerun with --leak-check=full to see details of leaked memory
==16470==
==16470== For counts of detected and suppressed errors, rerun with: -v
==16470== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 12 from 8)

Answer 1

Let's start by making this code just a lot easier to read (to my eye at least). I'd change:

const char str_GET[]       = "GET";

This statement, without the const keyword would take a constant char array (string), and copy it to str_GET. But do we really need that copy? Of course not, instead, a constant pointer to an immutable string will do fine. Your compiler might even optimize your code to do just that, but still, instead, I'd assign the address of this string to a constant pointer, and write:

const char *str_GET = "GET";

However, this will create these pointers each time the function is called. You could make them static, especially given the fact that the strings themselves won't change in between function calls, too.
For that very same reason, it would not be uncommon to see these string constants being defined as macro's, too.

Next:

char httpMethod[10] = {'\0'};
char *qString = (void *) 0;
char *host = (void *) 0;

Can be written as simple as this:

char httpMethod[10] = "",
    *qString = NULL,
    *host = NULL;
//or on a separate line:
char *ptr_SRV_CMD = NULL;

I'd consider the latter easier to read, too. Casting 0 to a void pointer isn't wrong, but an uninitialized, safe pointer is called a NULL-pointer. Assign it NULL, then.
In C, "" is the same as writing {'\0'}, only a lot shorter :).

Moving on

if (ptr_SRV_CMD = strstr (userData, str_GET)) {
    strncpy(httpMethod, str_GET, strlen(str_GET));
}
else if (ptr_SRV_CMD = strstr (userData, str_POST)) {
    strncpy(httpMethod, str_POST, strlen(str_POST));
}
else if (ptr_SRV_CMD = strstr (userData, str_DELETE)) {
    strncpy(httpMethod, str_DELETE, strlen(str_DELETE));
}
else if (ptr_SRV_CMD = strstr (userData, str_PUT)) {
    strncpy(httpMethod, str_PUT, strlen(str_PUT));
}

Fine, though since httpMethod is already initialized to an empty string, you could use strncat here, too. However, using all those strlen calls really is adding quite pointless overhead. You know httpMethod is an empty string, and you know its size to be 10, so why not use a constant max length? optionally defined as macro:

#define HTTP_METHOD_LEN 10

char httpMethod[HTTP_METHOD_LEN] = "";
//later on:
strncat(httpMethod, str_PUT, HTTP_METHOD_LEN-1);

In theory, you could even do away with that -1 at the end, simply because you know the source string (str_POST, str_GET and the like) will always fit in httpMethod.
With that in mind, you could use:

strcpy(httpMethod, str_POST);

quite safely. It's bound to be faster, anyway.
Anyway, next:

if (ptr_SRV_CMD == NULL)
    return -1;

Why aren't you putting this in an else branch along with the previous if-else if blocks? I mean: ptr_SRV_CMD won't be NULL unless the substring in question wasn't found, right? then wouldn't this make more sense:

if (ptr_SRV_CMD = strstr (userData, str_GET)) {
    strncpy(httpMethod, str_GET, strlen(str_GET));
}
else if (ptr_SRV_CMD = strstr (userData, str_POST)) {
    strncpy(httpMethod, str_POST, strlen(str_POST));
}
else if (ptr_SRV_CMD = strstr (userData, str_DELETE)) {
    strncpy(httpMethod, str_DELETE, strlen(str_DELETE));
}
else if (ptr_SRV_CMD = strstr (userData, str_PUT)) {
    strncpy(httpMethod, str_PUT, strlen(str_PUT));
}
else//<-ptr_SRV_CMD is null here
    return -1;

Then you have 2 very similar blocks of code, perhaps consider writing a function for this bit, but change the code a little. I've put some remarks in comments

char *ptr_HTTP = strstr(userData, str_HTTP);
//no need to check for ptr_SRV_CMD being a null pointer again here...
if (ptr_HTTP && ptr_SRV_CMD) {
    char *ptr_qs_start = ptr_SRV_CMD + strlen(httpMethod) + 1;
    //This HAS to be true for this block to do anything
    //move it to the first if
    if (ptr_HTTP > ptr_qs_start) {
        //guaranteed to be positive at this point, perhaps use size_t?
        int qsSize = ptr_HTTP - ptr_qs_start;
        //malloc + memset == calloc
        qString = malloc(qsSize + 1);
        memset(qString, '\0', (qsSize + 1));
        strncpy(qString, ptr_qs_start, qsSize);
    }
}

So, what I'd write is this:

char *ptr_HTTP = NULL;//
if ((ptr_HTTP = strstr(userData, str_HTTP)) != NULL
   && ptr_HTTP > (ptr_SRV_CMD + strlen(httpMethod) + 1))
{
    int qsSize = ptr_HTTP - (ptr_SRV_CMD + strlen(httpMethod) + 1);
    qString = calloc(qsSize, sizeof *qString);
    strncpy(qString, ptr_qs_start, qsSize);
}

A couple of remarks after a quick glance at your code, more to come shortly .

Update - recommendation for future development
As 200_success rightfully pointed out in his answer: you're implementing some form of rudimentary parsing functionality that will work +95% of the time in your case. As time progresses, however, and your project grows, it's not unlikely for you to find yourself having to constantly return to this code, add some functionality and fix new bugs all the time.

For that reason, learning to use a parser lib could well be worth your while. In order for you to be able to keep using your structs, just write a simple wrapper around the lib you choose.
You can then separate that code, compile and link it independently, which will make it easier to maintain, too.

Here's a link to a decent lib for you to use, but google around, there's many to choose from.

Something else I'd like to say, even though this is "risky advice": don't blindly rely on mem-leaks reported by valgrind. Valgrind is a mature tool, of course, and it's very valuable, but false positives aren't unheard of.
Besides, unfreed pointers are, in many (if not most) cases not the cause of leaks. An unclosed file pointer, for example is far more likely to cause leaks.
In fact, after a quick search this came up, read the answer + links. It talks about what the job of free actually is, and how it rarely returns memory to the OS. It also explicitly states that you shouldn't call free at the end of your program. By That, they don't mean: it's bad of you to do so, but they're saying that: it shouldn't be required of you, the OS should be able to reclaim the memory.

And indeed, most modern OS's will in fact reclaim un-freed pointers the moment a process terminates. When those pointers can in fact be NULL pointers, then it's also best to check before calling free.
Perhaps you've heard this before, but calling free on an uninitialized pointer is dangerous, and outright wrong. Calling free on a NULL pointer is deemed safe (as in no undefined behaviour), but it's a pointless function call. Choosing to write something in C often implies writing time-critical applications, so any pointless function call is to be avoided.
Couple that to what I've said before: most OS's being able to reclaim unfreed memory and you will, I hope, understand that I would alter code like this:

int main ( void )
{
    char * my_buffer = malloc(20000 * sizeof *my_buffer);
    //do stuff
    if (request_clear_buffer())
        free(my_buffer);
    //do more stuff
    free(my_buffer);//<== DANGER
    return 0;
}

To look like this, when I get my hands on it:

int main ( void )
{
    char * my_buffer = malloc(20000 * sizeof *my_buffer);
    //do stuff
    if (my_buffer && request_clear_buffer() == true)
    {
        free(my_buffer);
        my_buffer = NULL;//<-- safety first
    }
    //do more stuff
    /**
     * Uncomment if you want, but free before return on our system
     * is pointless, system will reclaim memory just fine
     *
     * if (my_buffer) free(my_buffer);
     */
    return 0;
}

Of course, if this code is ever to be used in other projects, and I don't know what system it'll be running under, I will uncomment the code. But on most OS's, there really is no need to do so.

You may be wondering why I said this to be risky advice, and why people keep saying how "every malloc/calloc/realloc call must be accompanied by a free call". That's simple: it's not because, at some point you need to allocate memory that that chunk of memory will be required for as long as your program runs.
The reason why free is an important function is because it allows you, as programmer, to use heap memory, and release it as soon as you're done playing with it. If you never do so, the program's memory usage will just grow and grow, right up to the point where either the heap is depleted or the program exits. Only at that point, all of the memory you claimed will be freed again.
That's why malloc without free is deemed bad practice.

Answer 2

Ad hoc string searching is a poor substitute for parsing the headers according to RFC 2616. That is, you should split the request by CRNL line breaks. For the first line, look for three space-separated words. On subsequent lines, look for a field name, followed by a :, followed by the field value.

As currently written, your code could get fooled by a request that looks like:

POST /KFC/CHICKEN_NUGGETS.jpg HTTP/1.0
Host: example.com

or by

GET /wiki/List_of_HTTP_status_codes HTTP/1.0
Host: en.wikipedia.org

---

What you call the "Query String", the RFC calls the "URI". RFC 3986 defines the Query String to be the part of the URI that follows a ? and precedes a # (if it exists). To avoid confusion, you should use the commonly accepted vocabulary.