You (potentially, depending on which branches are taken) use malloc twice but never free anything.
–
YuushiApr 8 '14 at 7:02

yea I could figure out that, I put freed them after using. now leak is gone. Please give me the feedback about the function. Can it be done simpler than this?
–
BlueBirdApr 8 '14 at 7:06

@BlueBird: You aren't freeing host and qString before the return, are you? because then you'd have undefined behaviour on your hands: the memory the struct members point to, then, would be freed...
–
Elias Van OotegemApr 8 '14 at 8:22

2 Answers
2

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.

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 :).

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 knowhttpMethod is an empty string, and you know its size to be 10, so why not use a constant max length? optionally defined as macro:

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:

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.

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:

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.

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.

Nips, I didn't get that memo. My answers tend to be quite verbose, I know. However your point was too important not to mention in my answer, too. hence the +1, but I'll stop editing my answer meanwhile before it's long enough to be published as a booklet
–
Elias Van OotegemApr 8 '14 at 10:56