597684 - Use Assoc-Req HTTP response header to validate Pipelined Responses

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

draft-nottingham-http-pipeline-00.txt defines a simple mechanism for servers to identify the request that was used to generate a response - the idea is that a pipelining client can use this information to identify any breakage along the transport path.

As I build out a more sophisticated http pipelining infrastructure any of these kinds of failures can provide an input into that system. For now this patch makes any pipelined transaction that fails this test throw an error and mark the connection info object as non pipeline capable. Marking CI in that way will prevent pipelines with that server until the CI goes away - which is rather more ephemeral than we want long term :) as part of this it introduces the concept of "banned pipelining" on CI which cannot be reverted by means of reading a simple pipeline eligible transaction the way "supports pipelining = false" can be reverted.

The patch depends on the patches for bugs 447866 and 232030

It contains a disabled test - it is disabled at the moment because I cannot figure out how to make the unit test framework submit pipelines.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: Implement Assoc-req and Banned Pipelines on CI

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: Implement Assoc-req and Banned Pipelines on CI

update bitrot, confrom better to style guide, etc..

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: Implement Assoc-req and Banned Pipelines on CI v 4

update for logging and bitrot

Comment 4

[Honza Bambas (:mayhemer)]

Jason, stealing review here.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 513669 [details] [diff] [review]
Implement Assoc-req and Banned Pipelines on CI v 4

>diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
>+        rv = EnsureAssocReq();
>+        if (NS_FAILED(rv))
>+            return rv;

Maybe do this check before establishing the MD5 check stream?  You can potentially save resources.

>+nsHttpChannel::EnsureAssocReq()
>+    if (assoc_val && mURI) {

Maybe just return NS_OK on inverted condition? The new code will be in general cleaner and more readable with early returns then with large indention blocks if possible.

>+        nsRefPtr<nsStandardURL> assoc_url = new nsStandardURL();
>+        if (assoc_url) {

I am not sure you need this check anymore (infallible new)

>+            method = net_FindCharNotInSet(assoc_val, HTTP_LWS);
>+            assoc_val = nsnull;
>+            if (method) {
>+                endofmethod = net_FindCharInSet(method, HTTP_LWS);
>+                if (endofmethod)
>+                    assoc_val = net_FindCharNotInSet(endofmethod, HTTP_LWS);
>+                if (assoc_val) {

Another place we would use a lexical analyzer...  If we had any...

>+                    // check the URL
>+                    mURI->SchemeIs("https", &usingSSL);
>+                    port = usingSSL ? 443 : 80;
>+                    assoc_val_str.AssignASCII(assoc_val);
>+                    rv = assoc_url->Init(nsIStandardURL::URLTYPE_AUTHORITY,
>+                                         port, assoc_val_str,
>+                                         nsnull, nsnull);

Did you have any strong reason not to use just NS_NewURI(assoc_url) here?  It can save you few lines and is more correct IMO.

>+    return NS_OK;

Just to confirm, you are actually ignoring the header that is any way broken, right?  I don't see any suggestions in the spec on it, so it is probably OK.

>@@ -4028,16 +4103,18 @@ nsHttpChannel::OnStopRequest(nsIRequest 
>                     if (!md5B64.Equals(normalizedHttpMD5)) {
>+                        if (mConnectionInfo)
>+                            mConnectionInfo->BanPipelining();

Hmm.  More a rhetorical question: why isn't this new BanPipelining API present in a patch (maybe even separate) lower in the patch queue?

>diff --git a/netwerk/protocol/http/nsHttpConnectionInfo.cpp b/netwerk/protocol/http/nsHttpConnectionInfo.cpp
>+PRBool
>+nsHttpConnectionInfo::SetSupportsPipelining(PRBool support)
>+{
>+    if (!mBannedPipelining)
>+        mSupportsPipelining = support;
>+    return mSupportsPipelining;
>+}

Matter of a personal taste - I would expect here (more readable and safer: make 100% sure we are banned when the flag gets in any way screwed):

+    if (mBannedPipelining)
+        return PR_FALSE;
+
+    return mSupportsPipelining;

>diff --git a/netwerk/protocol/http/nsHttpPipeline.cpp b/netwerk/protocol/http/nsHttpPipeline.cpp
> nsHttpPipeline::AddTransaction(nsAHttpTransaction *trans)
>+    PRInt32 qlen = mRequestQ.Length();
>+    
>+    if (qlen != 1)
>+        trans->SetPipelineUsed();

Please explain why not to do this also for the first (or a lonely) added transaction.

If I remember correctly, when the connection serving the pipeline is closed, we are restarting all the transactions, right?  I believe you have to drop the flag for them.

So I was thinking of changing your API a following way:
- have a flag bool nsAHttpConnection::IsPipeline() ; true in the pipeline implementation, false in the connection impl ; not very nice either but could have usage also for other cases we need to recognize the object
- that flag would be read by a transaction when the connection is set on it to ensure we have access to the flag even after the transaction has been closed

This ensures you a perfect state of the flag and is also cleaner then adding the two methods nsHttpPipeline::SetPipelineUsed and nsHttpPipeline::PipelineUsed that are empty.

>diff --git a/netwerk/test/unit/test_assoc.js b/netwerk/test/unit/test_assoc.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit/test_assoc.js
>@@ -0,0 +1,76 @@
>+do_load_httpd_js();
>+
>+var httpserver = new nsHttpServer();
>+var index = 0;

Maybe rename to currentTestIndex?

>+var tests = [
>+             // this is valid
>+             {url: "/assoc/assoctest?valid",
>+             responseheader: [ "Assoc-Req: GET http://localhost:4444/assoc/assoctest?valid" ],
>+             flags : 0},
>+
>+             // this is invalid because the method is wrong
>+             {url: "/assoc/assoctest?ivalid",
>+             responseheader: [ "Assoc-Req: POST http://localhost:4444/assoc/assoctest?ivalid" ],
>+             flags : CL_EXPECT_LATE_FAILURE},
>+             
>+             // this is invalid because the url is wrong
>+             {url: "/assoc/assoctest?notvalid",
>+              responseheader: [ "Assoc-Req: GET http://localhost:4444/wrongpath/assoc/assoctest?notvalid" ],
>+              flags : CL_EXPECT_LATE_FAILURE},
>+];

Maybe also add tests for a broken Assoc-Req header values, e.g. 
- no space or more then just a single space between method and URL
- case like "GEThttp://example.com/ " (with a trailing space(s), not sure we trim header values automatically)
- spaces and optional other stuff after the URL

The parsing code seems OK, but have at least some test against any simply revealable buffer overflows would be good.

>+function setupChannel(url)
>+    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);

Is the QI() really needed?

>+function startIter()
>+{
>+    var channel = setupChannel(tests[index].url );

A space before the closing bracket.

>+function run_test()
>+{
>+    // disable this test as Assoc-Req is not evaluated unless the transaction is
>+    // returned on a pipeline
>+    return;

Please just check it is not mandatory for a test to do at least one do_check_*() call.

Comment 6

[Patrick McManus [:mcmanus]]

> 
> Just to confirm, you are actually ignoring the header that is any way broken,
> right?  I don't see any suggestions in the spec on it, so it is probably OK.

confirmed.

> 
> Please explain why not to do this also for the first (or a lonely) added
> transaction.

eventually this property morphs into an int representing the depth of the pipeline that this transaction was scheduled on. we use that as feedback in order to "wade-in" to more aggressive pipelines - at first we want to see some simple short ones succeed before going in all the way. A lonely transaction doesn't make the cut.


> If I remember correctly, when the connection serving the pipeline is closed, we
> are restarting all the transactions, right?  I believe you have to drop the
> flag for them.

that's a really good point. I had that code in a later patch, but you're right that is necessary here so I have moved it forward.

> 
> Maybe also add tests for a broken Assoc-Req header values, e.g. 
> - no space or more then just a single space between method and URL
> - spaces and optional other stuff after the URL

unfortunately the test harness send header code normalizes all the spaces away.. I added some more tests, but not those.

I was able to enable the tests by default, even in the absence of a pipelining client by hooking a pragma that indicates we should evaluate assoc-req even in the absense of a pipeline.

I also changed the enforcement of a failed assoc-req to match that of md. i.e. log and pipeline-feedback on failure, but don't fail the HTTP transaction unless a pref is set for it.

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: Implement Assoc-req and Banned Pipelines on CI v5

updated with review comments

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: implement Assoc-req and Banned Pipelines on CI v6

undo bitrot to reflect larch

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus from comment #6)
> unfortunately the test harness send header code normalizes all the spaces
> away.. I added some more tests, but not those.

You can do seizePower on the request in the js handler and then write what you want, including the headers.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 542080 [details] [diff] [review]
implement Assoc-req and Banned Pipelines on CI v6

Review of attachment 542080 [details] [diff] [review]:
-----------------------------------------------------------------

This probably needs to be updated according the current deal on the patch queue (bug 659760 comment 2).

According the test, I'd really see more tests for the broken header (see comment 9) like two spaces at the start, space+method+space only, two spaces between method and url, not valid url, method of the same length but different letter(s), different URL encoding and escaping.  I can update the test my self, actually, if you're out of time.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1826,5 @@
> +    
> +    // check the URL
> +    nsCOMPtr<nsIURI> assoc_url;
> +    NS_NewURI(getter_AddRefs(assoc_url), assoc_val);
> +    if (!assoc_url)

Maybe better to check on the result of NS_NewURI.

@@ +1829,5 @@
> +    NS_NewURI(getter_AddRefs(assoc_url), assoc_val);
> +    if (!assoc_url)
> +        return NS_OK;
> +
> +    mURI->Equals(assoc_url, &equals);

I hope there is no way for a different encoding of mURI and assoc_url that would confuse Equals().

Is there said in what form the URL in Assoc-Req has to be present?  IDN, escaping...

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +127,5 @@
> +        trans->SetPipelineUsed(qlen);
> +    else
> +        // do it for this case in case an idempotent cancellation
> +        // is being repeated and an old value needs to be cleared
> +        trans->SetPipelineUsed(0);

Please all commands in braces (for both true and false cases).

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #10)
> Comment on attachment 542080 [details] [diff] [review]
> implement Assoc-req and Banned Pipelines on CI v6
> 
> Review of attachment 542080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This probably needs to be updated according the current deal on the patch
> queue (bug 659760 comment 2).

I'm not sure what you're saying here.


> According the test, I'd really see more tests for the broken header (see
 
I think you should file a followup bug and if you'd take it I would be appreciative. I think the basics are covered here and test coverage for the corner cases of a fringe feature probably isn't the best priority right now.

> I hope there is no way for a different encoding of mURI and assoc_url that
> would confuse Equals().
> 
> Is there said in what form the URL in Assoc-Req has to be present?  IDN,
> escaping...

it is defined by the same request uri format from ietf-httpbis-p1-messaging, so they should match.

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: patch v7

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 596706 [details] [diff] [review]
patch v7

Review of attachment 596706 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

r=honzab

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +130,5 @@
>          mUtilizedPipeline = true;
>  
>      NS_ADDREF(trans);
>      mRequestQ.AppendElement(trans);
> +    PRInt32 qlen = mRequestQ.Length();

nsTArray::Length is of size_type type that is unsigned (PRUint32).  I think I've already mentioned this in some comment.  No need for qlen and PipelinePostion be signed, unless you have a strong reason (like -1 flagging).

Comment 14

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3e5881da7b

Comment 15

[Julian Reschke]

Seems it's time to write a spec :-).

Comment 16

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because something in the push appeared to cause a leak in mochitest-browser-chrome:
https://hg.mozilla.org/integration/mozilla-inbound/rev/577da08878cb

Comment 17

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6f1cca200c

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/fd6f1cca200c