Re: [PATCH]: Sending HTTP_BAD_REQUEST to client in case of bad request

Hi,

how about attached patch? it should work ok but untested.

Thanks,
Alex

Dziugas Baltrunas wrote:

> Hi,
> 
> ok, what about putting simple return statement instead of goto error?
> 
> In case of queued for sending, http_send_reply() will register a
> callback to receive_request() with a state sending_reply which will
> close the connection afterwards. So it means that http_send_reply()
> will destroy the connection anyway.
> 
> However, above will not work, i.e. client will get a 400 reply but the
> connection won't be closed because we have p->persistent_conn = 1 as
> default in client_create() which I think has to be changed into 0. Who
> said that HTTP 1.1 connection should default to keep-alive anyway?
> 
> Btw, in order for others to understand what this is all about, given
> that localhost:13013 is your smsbox up and running, try the following:
> 
>> telnet localhost 13013
> Trying 127.0.0.1...
> Connected to localhost.
> Escape character is '^]'.
> asdf
> Connection closed by foreign host.
> 
> Which means that it violates http, gives no indication to client, etc.
> etc., so this should definitely be changed. Proposed way is the
> following:
> 
>> telnet localhost 13013
> Trying 127.0.0.1...
> Connected to localhost.
> Escape character is '^]'.
> asdf
> HTTP/1.1 400 Bad Request
> Server: Kannel/cvs-20060308
> Content-Length: 0
> 
> Connection closed by foreign host.
> 
> On 3/23/06, Alexander Malysh <[email protected]> wrote:
>> Hi,
>>
>> no go with your patch. http_send_reply is a queuing function. that means
>> when response could not be sent at once (e.g. client too slow) response
>> sending will be registered (queued) in fdset. but in your patch you will
>> destroy httpclient with goto error statement.
>>
>> You should split http_send_reply function in a helper function which
>> just prepare response and main function that send response queued as
>> earlier. Then here instead of sending through http_send_reply you do
>> something like:
>>
>> Octstr *resp = http_prepare_reply(...)
>> conn_send(resp);
>> goto error;
>>
>> Thanks,
>> Alex
>>
>> Dziugas Baltrunas schrieb:
>> > Hi list,
>> >
>> > in case of client sends us malformed URL (such as
>> > /cgi-bin/sendsms?user=test&pass=test&to=12345 &text=test), attached
>> > patch sends a HTTP 400 Bad request error instead of simply closing the
>> > socket thus giving no indication to the client.
>> >
>> > --
>> > Dziugas
>> >
>> >
>> >

Index: gwlib/http.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/http.c,v
retrieving revision 1.240
diff -a -u -p -r1.240 http.c
--- gwlib/http.c	5 Mar 2006 14:37:26 -0000	1.240
+++ gwlib/http.c	23 Mar 2006 18:52:14 -0000
@@ -2097,8 +2097,18 @@ static void receive_request(Connection *
 	    ret = parse_request_line(&client->method, &client->url,
                                      &client->use_version_1_0, line);
 	    octstr_destroy(line);
-	    if (ret == -1)
-	    	goto error;
+            /* client sent bad request? */
+	    if (ret == -1) {
+                /*
+                 * mark client as not persistent in order to destroy connection
+                 * afterwards
+                 */
+                client->persistent_conn = 0;
+                /* unregister connection, http_send_reply handle this */
+                conn_unregister(conn);
+                http_send_reply(client, HTTP_BAD_REQUEST, NULL, NULL);
+                return;
+            }
    	    /*
 	     * RFC2616 (4.3) says we should read a message body if there
 	     * is one, even on GET requests.