Repository: netty/netty
Issue: Adding DefaultHttpHeaders to itself creates infinite loop
Labels: defect

Example:

```
public void test() {
    HttpHeaders headers = new DefaultHttpHeaders();
    headers.add("foo", "bar");
    headers.add(headers);
    // This will never end
    headers.forEach(entry -> {});
}
```


Issue comments:
...why would you even do that

@willisblackburn thanks for the report. Will add a guard against this

@willisblackburn - I'm assuming you are using Netty 4.0?


Pull request review comments:
In 4.1 we are not consistent in that this method `returns this;` and `add` throws an exception. I think we should be consistent in how we behave across versions (and maybe how we react to these 2 situations). So do we want to throw an exception, or just `return this;`?

hmm. when adding this check my thought process was that this should only be triggered by a coding error (i.e. couldn't think of a reason why one would need to do this).

either way is fine for me. I don't care deeply about this :-). Should be documented either way though.

+1. I lean toward only throw exception if the semantics of the interface cannot be preserved. If `return this;` preserves semantics then no need for exception.

Ok so only for `add(...)` fine by me as well. Not that I think calling set with itself makes any sense though

I'm leaning toward throwing an exception in both cases, since it is a programming error and you want to be explicit.  Not throwing may lead to buggy application code.   That said, I don't feel strongly.

@Scottmitch so what ? @nmittler agree's with my original patch :)

LOL ... yeah, what he said!!!! (grrrrrr)

@normanmaurer @nmittler - Well now things get interesting :)

I'm not strong on this either and it seems like no one is so @normanmaurer why don't you decide and just make sure we handle it consistently across versions (and documented).

Just to clarify the programming error statement ... the following code (and any code that calls `set` where it may take the same object) has a programming error:

``` java
HttpHeaders a = ...;
doSomething(a, someMethodWhichMayReturnParamter(a));

void doSomething(HttpHeaders a, HttpHeaders b) {
  if (something happens) {
    // Programming error because no check to a != b?
    a.set(b);
  }
}
```

Seems strange if we could just handle it with no side effects and not require the check (both internally to `set` and in the callers of `set`)

@Scottmitch yes that was the idea... 


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
index 2b706565083..3b3d9eca61b 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
@@ -59,6 +59,9 @@ void validateHeaderName0(CharSequence headerName) {
     @Override
     public HttpHeaders add(HttpHeaders headers) {
         if (headers instanceof DefaultHttpHeaders) {
+            if (headers == this) {
+                throw new IllegalArgumentException("can't add to itself.");
+            }
             DefaultHttpHeaders defaultHttpHeaders = (DefaultHttpHeaders) headers;
             HeaderEntry e = defaultHttpHeaders.head.after;
             while (e != defaultHttpHeaders.head) {
@@ -74,6 +77,9 @@ public HttpHeaders add(HttpHeaders headers) {
     @Override
     public HttpHeaders set(HttpHeaders headers) {
         if (headers instanceof DefaultHttpHeaders) {
+            if (headers == this) {
+                throw new IllegalArgumentException("can't add to itself.");
+            }
             clear();
             DefaultHttpHeaders defaultHttpHeaders = (DefaultHttpHeaders) headers;
             HeaderEntry e = defaultHttpHeaders.head.after;


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpHeadersTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpHeadersTest.java
index 47302c7425d..e2eadc33cab 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpHeadersTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpHeadersTest.java
@@ -67,4 +67,16 @@ public void testSetNullHeaderValueNotValidate() {
         HttpHeaders headers = new DefaultHttpHeaders(false);
         headers.set("test", (CharSequence) null);
     }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testAddSelf() {
+        HttpHeaders headers = new DefaultHttpHeaders(false);
+        headers.add(headers);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testSetSelf() {
+        HttpHeaders headers = new DefaultHttpHeaders(false);
+        headers.set(headers);
+    }
 }