Skip to content

(#42) HtHead can be broken. #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 23, 2019
Merged

(#42) HtHead can be broken. #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 16 additions & 47 deletions src/main/java/org/cactoos/http/HtHead.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@
package org.cactoos.http;

import java.io.InputStream;
import java.io.SequenceInputStream;
import java.nio.charset.Charset;
import java.util.Scanner;
import org.cactoos.Input;
import org.cactoos.io.DeadInputStream;
import org.cactoos.io.InputStreamOf;

/**
Expand All @@ -38,9 +38,14 @@
public final class HtHead implements Input {

/**
* Buffer length.
* Header separator.
*/
private static final int LENGTH = 16384;
private static final String DELIMITER = "\r\n\r\n";

/**
* Charset that is used to read headers.
*/
private static final Charset CHARSET = Charset.defaultCharset();

/**
* Response.
Expand All @@ -56,50 +61,14 @@ public HtHead(final Input rsp) {
}

@Override
@SuppressWarnings("PMD.AvoidInstantiatingObjectsInLoops")
public InputStream stream() throws Exception {
final InputStream stream = this.response.stream();
final byte[] buf = new byte[HtHead.LENGTH];
InputStream head = new DeadInputStream();
while (true) {
final int len = stream.read(buf);
if (len < 0) {
break;
}
final int tail = HtHead.findEnd(buf, len);
final byte[] temp = new byte[tail];
System.arraycopy(buf, 0, temp, 0, tail);
head = new SequenceInputStream(head, new InputStreamOf(temp));
if (tail != len) {
break;
}
try (final Scanner scanner = new Scanner(
this.response.stream(),
HtHead.CHARSET.name()
)) {
scanner.useDelimiter(HtHead.DELIMITER);
return new InputStreamOf(scanner.next());
}
return head;
}

/**
* Find header end.
* @param buf Buffer where to search
* @param len Size of the buffer
* @return End of the header
*/
private static int findEnd(final byte[] buf, final int len) {
final byte[] end = {'\r', '\n', '\r', '\n'};
int tail = end.length - 1;
while (tail < len) {
boolean found = true;
for (int num = 0; num < end.length; ++num) {
if (end[num] != buf[tail - end.length + 1 + num]) {
found = false;
break;
}
}
if (found) {
tail = tail - end.length + 1;
break;
}
++tail;
}
return tail;
}
}

90 changes: 72 additions & 18 deletions src/test/java/org/cactoos/http/HtHeadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,20 @@
*/
package org.cactoos.http;

import java.io.IOException;
import java.util.Random;
import org.cactoos.Text;
import org.cactoos.io.BytesOf;
import org.cactoos.io.InputOf;
import org.cactoos.text.JoinedText;
import org.cactoos.text.RepeatedText;
import org.cactoos.text.ReplacedText;
import org.cactoos.text.TextOf;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.hamcrest.core.IsEqual;
import org.junit.Test;
import org.llorllale.cactoos.matchers.Assertion;
import org.llorllale.cactoos.matchers.EndsWith;
import org.llorllale.cactoos.matchers.StartsWith;
import org.llorllale.cactoos.matchers.TextHasString;

/**
Expand All @@ -45,9 +50,10 @@
public final class HtHeadTest {

@Test
public void takesHeadOutOfHttpResponse() throws IOException {
MatcherAssert.assertThat(
new TextOf(
public void takesHeadOutOfHttpResponse() {
new Assertion<>(
"Header does not have 'text/plain'",
() -> new TextOf(
new HtHead(
new InputOf(
new JoinedText(
Expand All @@ -59,15 +65,16 @@ public void takesHeadOutOfHttpResponse() throws IOException {
)
)
)
).asString(),
Matchers.endsWith("text/plain")
);
),
new EndsWith("text/plain")
).affirm();
}

@Test
public void emptyHeadOfHttpResponse() throws IOException {
MatcherAssert.assertThat(
new TextOf(
public void emptyHeadOfHttpResponse() {
new Assertion<>(
"Text does not have an empty string",
() -> new TextOf(
new HtHead(
new InputOf(
new JoinedText(
Expand All @@ -80,16 +87,17 @@ public void emptyHeadOfHttpResponse() throws IOException {
)
),
new TextHasString("")
);
).affirm();
}

@Test
public void largeText() throws IOException {
public void largeText() {
//@checkstyle MagicNumberCheck (1 lines)
final byte[] bytes = new byte[18000];
new Random().nextBytes(bytes);
MatcherAssert.assertThat(
new TextOf(
new Assertion<>(
"Header does not have text/plain header",
() -> new TextOf(
new HtHead(
new InputOf(
new JoinedText(
Expand All @@ -101,9 +109,55 @@ public void largeText() throws IOException {
)
)
)
).asString(),
Matchers.endsWith("text/plain")
);
),
new EndsWith("text/plain")
).affirm();
}

@Test
public void edgeOfTheBlockTearing() throws Exception {
final int size = 16384;
final Text header = new JoinedText(
"\r\n",
"HTTP/1.1 200 OK",
"Referer: http://en.wikipedia.org/wiki/Main_Page#\0",
"Content-type: text/plain",
""
);
final Text block = new ReplacedText(
header,
"\0",
new RepeatedText(
"x",
size - header.asString().length() + 1
).asString()
);
new Assertion<>(
"make sure the constructed block is exact size",
() -> block.asString().length(),
new IsEqual<>(
Copy link
Contributor

@victornoel victornoel Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyakharlamov please use cactoos-matchers' TextIs

Copy link
Contributor Author

@ilyakharlamov ilyakharlamov Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I am constructing a block and making sure it has the exact length of the block.
How TextIs can help to verify that the string is of a certain size?

Copy link
Contributor

@victornoel victornoel Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyakharlamov you are right yes. I think this test is too complex to be easily understandable.

Why are you checking the block length? You are not testing HtHead here, so I don't think you should be asserting anything. I would advise to remove this assertion.

Copy link
Contributor Author

@ilyakharlamov ilyakharlamov Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel In order to reproduce the issue, I need to have a header of the exact size of the block (16384 bytes)
So it's way too easy to miss one character, like one extra space and the block will be 16383 or 16385 and the test will yield a false-positive result. So here I am checking the length to make sure that the constructed header is exactly 16384 bytes before running the actual test.

Copy link
Contributor

@victornoel victornoel Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilyakharlamov ok, it's good for me then, thank you for the explanation

size
)
).affirm();
new Assertion<>(
String.format("Edge of the block tearing for size: %s", size),
() -> new TextOf(
new HtHead(
new InputOf(
new JoinedText(
"\r\n",
block.asString(),
"",
"body here"
)
)
)
),
Matchers.allOf(
new StartsWith("HTTP"),
new TextHasString("OK\r\nReferer"),
new EndsWith("text/plain")
)
).affirm();
}
}