From 1fae77c64d52615625772cc8b157579eb7bd1721 Mon Sep 17 00:00:00 2001 From: Alex Osborne Date: Tue, 31 Oct 2017 18:41:00 +0900 Subject: [PATCH 1/3] Enforce robots.txt character limit per char not per line Since we were only enforcing the size limit after reading each line a single very long line could cause us to to run out of memory. This change enforces the character limit on each read character not just on line boundaries. We also correct the count of processed characters in the warning message, which was not counting newline and linefeed characters. --- .../org/archive/modules/net/Robotstxt.java | 83 +++++++++++++++---- .../archive/modules/net/RobotstxtTest.java | 14 ++++ 2 files changed, 82 insertions(+), 15 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/net/Robotstxt.java b/modules/src/main/java/org/archive/modules/net/Robotstxt.java index b61f5822..cda1a17e 100644 --- a/modules/src/main/java/org/archive/modules/net/Robotstxt.java +++ b/modules/src/main/java/org/archive/modules/net/Robotstxt.java @@ -20,6 +20,7 @@ package org.archive.modules.net; import java.io.BufferedReader; import java.io.IOException; +import java.io.Reader; import java.io.Serializable; import java.util.HashMap; import java.util.LinkedList; @@ -80,28 +81,21 @@ public class Robotstxt implements Serializable { } protected void initializeFromReader(BufferedReader reader) throws IOException { + BoundedLineReader lineReader = new BoundedLineReader(reader, MAX_SIZE); String read; - long charCount = 0; // current is the disallowed paths for the preceding User-Agent(s) RobotsDirectives current = null; while (reader != null) { - // we count characters instead of bytes because the byte count isn't easily available - if (charCount >= MAX_SIZE) { - logger.warning("processed " + charCount + " characters, ignoring the rest (see HER-1990)"); - reader.close(); - reader = null; - continue; - } - do { - read = reader.readLine(); - if (read != null) { - charCount += read.length(); - } + read = lineReader.readLine(); // Skip comments & blanks - } while (read != null && charCount < MAX_SIZE - && ((read = read.trim()).startsWith("#") || read.length() == 0)); + } while (read != null && ((read = read.trim()).startsWith("#") || read.length() == 0)); if (read == null) { + if (lineReader.reachedLimit()) { + // we count characters instead of bytes because the byte count isn't easily available + logger.warning("processed " + lineReader.getCharsProcessed() + + " characters, ignoring the rest (see HER-1990)"); + } reader.close(); reader = null; } else { @@ -194,6 +188,65 @@ public class Robotstxt implements Serializable { } } + /** + * Read lines from a reader until a character limit is reached. + * + * Always returns whole lines. If the limit would cause a partial line to be + * read the data is discarded. + */ + private static class BoundedLineReader { + private final Reader reader; + private long remaining; + private long charsProcessed = 0; + + BoundedLineReader(Reader reader, long limit) { + this.reader = reader; + this.remaining = limit; + } + + String readLine() throws IOException { + StringBuilder buffer = new StringBuilder(); + + while (!reachedLimit()) { + int c = reader.read(); + + if (c < 0) { // end of file + if (buffer.length() > 0) { // file didn't end on a linefeed + charsProcessed += buffer.length(); + return buffer.toString(); + } else { + return null; + } + } + + remaining--; + + if (c == '\r' || c == '\n') { + charsProcessed += buffer.length() + 1; + return buffer.toString(); + } + + buffer.append((char) c); + } + + return null; + } + + boolean reachedLimit() { + return remaining <= 0; + } + + /** + * Returns the number of characters that have been read. + * + * Includes newline characters. + * Excludes any partial line data read before the limit is reached. + */ + long getCharsProcessed() { + return charsProcessed; + } + } + /** * Does this policy effectively allow everything? (No * disallows or timing (crawl-delay) directives?) diff --git a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java index 1f529077..132d5617 100644 --- a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java +++ b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java @@ -227,4 +227,18 @@ public class RobotstxtTest extends TestCase { assertEquals(99f, rt.getDirectivesFor("a").getCrawlDelay()); } + + public void testSizeLimit() throws IOException { + StringBuilder builder = new StringBuilder( + "User-agent: a\n" + + " Disallow: /\n" + + "User-Agent: b\n"); + for (int i = 0; i < Robotstxt.MAX_SIZE; i++) { + builder.append(' '); + } + builder.append("Disallow: /\n"); + Robotstxt rt = new Robotstxt(new BufferedReader(new StringReader(builder.toString()))); + assertFalse("we should parse the first part", rt.getDirectivesFor("a").allows("/foo")); + assertTrue("but ignore anything after the size limit", rt.getDirectivesFor("b").allows("/foo")); + } } From 68ceceedd29204d5a52c935aa6163fa45a8d96d4 Mon Sep 17 00:00:00 2001 From: Alex Osborne Date: Wed, 1 Nov 2017 09:29:28 +0900 Subject: [PATCH 2/3] Use simpler read 500k chars and split method suggested by @nlevitt While we temporarily use a little more memory this version is a lot less codes. It also allows us to do away with the BufferedReader. --- .../org/archive/modules/net/CrawlServer.java | 3 +- .../org/archive/modules/net/Robotstxt.java | 108 +++++------------- .../archive/modules/net/RobotstxtTest.java | 38 +++--- 3 files changed, 48 insertions(+), 101 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/net/CrawlServer.java b/modules/src/main/java/org/archive/modules/net/CrawlServer.java index 11aaf477..c223df19 100644 --- a/modules/src/main/java/org/archive/modules/net/CrawlServer.java +++ b/modules/src/main/java/org/archive/modules/net/CrawlServer.java @@ -174,10 +174,9 @@ public class CrawlServer implements Serializable, FetchStats.HasFetchStats, Iden InputStream contentBodyStream = null; try { - BufferedReader reader; contentBodyStream = curi.getRecorder().getContentReplayInputStream(); - reader = new BufferedReader(new InputStreamReader(contentBodyStream)); + InputStreamReader reader = new InputStreamReader(contentBodyStream); robotstxt = new Robotstxt(reader); validRobots = true; } catch (IOException e) { diff --git a/modules/src/main/java/org/archive/modules/net/Robotstxt.java b/modules/src/main/java/org/archive/modules/net/Robotstxt.java index cda1a17e..40d6e7a8 100644 --- a/modules/src/main/java/org/archive/modules/net/Robotstxt.java +++ b/modules/src/main/java/org/archive/modules/net/Robotstxt.java @@ -18,16 +18,17 @@ */ package org.archive.modules.net; -import java.io.BufferedReader; import java.io.IOException; import java.io.Reader; import java.io.Serializable; +import java.nio.CharBuffer; import java.util.HashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Pattern; import org.apache.commons.io.IOUtils; import org.archive.bdb.AutoKryo; @@ -43,7 +44,8 @@ public class Robotstxt implements Serializable { private static final Logger logger = Logger.getLogger(Robotstxt.class.getName()); - protected static final long MAX_SIZE = 500*1024; + protected static final int MAX_SIZE = 500*1024; + private static final Pattern LINE_SEPARATOR = Pattern.compile("\r\n|\r|\n"); // all user agents contained in this robots.txt // in order of declaration @@ -63,12 +65,16 @@ public class Robotstxt implements Serializable { public Robotstxt() { } - public Robotstxt(BufferedReader reader) throws IOException { - initializeFromReader(reader); + public Robotstxt(Reader reader) throws IOException { + try { + initializeFromReader(reader); + } finally { + IOUtils.closeQuietly(reader); + } } public Robotstxt(ReadSource customRobots) { - BufferedReader reader = new BufferedReader(customRobots.obtainReader()); + Reader reader = customRobots.obtainReader(); try { initializeFromReader(reader); } catch (IOException e) { @@ -80,25 +86,24 @@ public class Robotstxt implements Serializable { } } - protected void initializeFromReader(BufferedReader reader) throws IOException { - BoundedLineReader lineReader = new BoundedLineReader(reader, MAX_SIZE); - String read; + protected void initializeFromReader(Reader reader) throws IOException { + CharBuffer buffer = CharBuffer.allocate(MAX_SIZE); + while (buffer.hasRemaining() && reader.read(buffer) >= 0) ; + buffer.flip(); + + String[] lines = LINE_SEPARATOR.split(buffer); + if (buffer.limit() == buffer.capacity()) { + int processed = buffer.capacity() - lines[lines.length - 1].length(); + logger.warning("processed " + processed + " characters, ignoring the rest (see HER-1990)"); + // discard the partial line at the end so we don't process a truncated path + lines[lines.length - 1] = ""; + } + // current is the disallowed paths for the preceding User-Agent(s) RobotsDirectives current = null; - while (reader != null) { - do { - read = lineReader.readLine(); - // Skip comments & blanks - } while (read != null && ((read = read.trim()).startsWith("#") || read.length() == 0)); - if (read == null) { - if (lineReader.reachedLimit()) { - // we count characters instead of bytes because the byte count isn't easily available - logger.warning("processed " + lineReader.getCharsProcessed() + - " characters, ignoring the rest (see HER-1990)"); - } - reader.close(); - reader = null; - } else { + for (String read: lines) { + read = read.trim(); + if (!read.isEmpty() && !read.startsWith("#")) { // remove any html markup read = read.replaceAll("<[^>]+>",""); int commentIndex = read.indexOf("#"); @@ -188,65 +193,6 @@ public class Robotstxt implements Serializable { } } - /** - * Read lines from a reader until a character limit is reached. - * - * Always returns whole lines. If the limit would cause a partial line to be - * read the data is discarded. - */ - private static class BoundedLineReader { - private final Reader reader; - private long remaining; - private long charsProcessed = 0; - - BoundedLineReader(Reader reader, long limit) { - this.reader = reader; - this.remaining = limit; - } - - String readLine() throws IOException { - StringBuilder buffer = new StringBuilder(); - - while (!reachedLimit()) { - int c = reader.read(); - - if (c < 0) { // end of file - if (buffer.length() > 0) { // file didn't end on a linefeed - charsProcessed += buffer.length(); - return buffer.toString(); - } else { - return null; - } - } - - remaining--; - - if (c == '\r' || c == '\n') { - charsProcessed += buffer.length() + 1; - return buffer.toString(); - } - - buffer.append((char) c); - } - - return null; - } - - boolean reachedLimit() { - return remaining <= 0; - } - - /** - * Returns the number of characters that have been read. - * - * Includes newline characters. - * Excludes any partial line data read before the limit is reached. - */ - long getCharsProcessed() { - return charsProcessed; - } - } - /** * Does this policy effectively allow everything? (No * disallows or timing (crawl-delay) directives?) diff --git a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java index 132d5617..559bd7d6 100644 --- a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java +++ b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java @@ -20,6 +20,7 @@ package org.archive.modules.net; import java.io.BufferedReader; import java.io.IOException; +import java.io.Reader; import java.io.StringReader; import java.nio.ByteBuffer; @@ -29,7 +30,7 @@ import org.archive.bdb.AutoKryo; public class RobotstxtTest extends TestCase { public void testParseRobots() throws IOException { - BufferedReader reader = new BufferedReader(new StringReader("BLAH")); + Reader reader = new StringReader("BLAH"); Robotstxt r = new Robotstxt(reader); assertFalse(r.hasErrors); assertEquals(0,r.getNamedUserAgents().size()); @@ -57,8 +58,7 @@ public class RobotstxtTest extends TestCase { } static Robotstxt sampleRobots1() throws IOException { - BufferedReader reader = new BufferedReader( - new StringReader( + Reader reader = new StringReader( "User-agent: *\n" + "Disallow: /cgi-bin/\n" + "Disallow: /details/software\n" + @@ -78,13 +78,12 @@ public class RobotstxtTest extends TestCase { "Disallow: /\n" + "Crawl-Delay: 20\n"+ "Allow: /images/\n" - )); + ); return new Robotstxt(reader); } Robotstxt whitespaceFlawedRobots() throws IOException { - BufferedReader reader = new BufferedReader( - new StringReader( + Reader reader = new StringReader( " User-agent: *\n" + " Disallow: /cgi-bin/\n" + " Disallow: /details/software\n" + @@ -100,7 +99,7 @@ public class RobotstxtTest extends TestCase { " Disallow: /\n" + " Crawl-Delay: 20\n"+ " Allow: /images/\n" - )); + ); return new Robotstxt(reader); } @@ -144,8 +143,7 @@ public class RobotstxtTest extends TestCase { } Robotstxt htmlMarkupRobots() throws IOException { - BufferedReader reader = new BufferedReader( - new StringReader( + Reader reader = new StringReader( "\n" +"\n" +"/robots.txt\n" @@ -157,7 +155,7 @@ public class RobotstxtTest extends TestCase { +"\n" +"\n" +"\n" - )); + ); return new Robotstxt(reader); } @@ -188,7 +186,7 @@ public class RobotstxtTest extends TestCase { "Disallow:/service\n"; StringReader sr = new StringReader(TEST_ROBOTS_TXT); - Robotstxt rt = new Robotstxt(new BufferedReader(sr)); + Robotstxt rt = new Robotstxt(sr); { RobotsDirectives da = rt.getDirectivesFor("a", false); RobotsDirectives db = rt.getDirectivesFor("b", false); @@ -216,7 +214,7 @@ public class RobotstxtTest extends TestCase { + "User-agent: a\n" + "Crawl-delay: 99\n"; StringReader sr = new StringReader(TEST_ROBOTS_TXT); - Robotstxt rt = new Robotstxt(new BufferedReader(sr)); + Robotstxt rt = new Robotstxt(sr); assertFalse(rt.getDirectivesFor("a").allows("/foo")); @@ -231,14 +229,18 @@ public class RobotstxtTest extends TestCase { public void testSizeLimit() throws IOException { StringBuilder builder = new StringBuilder( "User-agent: a\n" + - " Disallow: /\n" + - "User-Agent: b\n"); + " Disallow: /\n" + + "User-Agent: b\nDisallow: /"); for (int i = 0; i < Robotstxt.MAX_SIZE; i++) { builder.append(' '); } - builder.append("Disallow: /\n"); - Robotstxt rt = new Robotstxt(new BufferedReader(new StringReader(builder.toString()))); - assertFalse("we should parse the first part", rt.getDirectivesFor("a").allows("/foo")); - assertTrue("but ignore anything after the size limit", rt.getDirectivesFor("b").allows("/foo")); + builder.append("\nUser-Agent: c\nDisallow: /\n"); + Robotstxt rt = new Robotstxt(new StringReader(builder.toString())); + assertFalse("we should parse the first few lines", + rt.getDirectivesFor("a").allows("/foo")); + assertTrue("ignore the line that breaks the size limit", + rt.getDirectivesFor("b").allows("/foo")); + assertTrue("and also ignore any lines after the size limit", + rt.getDirectivesFor("c").allows("/foo")); } } From d6d7f1b1628ecb73e6a1278fd4d0f0161834a925 Mon Sep 17 00:00:00 2001 From: Alex Osborne Date: Wed, 1 Nov 2017 10:04:02 +0900 Subject: [PATCH 3/3] Fix ArrayIndexOutOfBoundsException when all lines are blank --- .../main/java/org/archive/modules/net/Robotstxt.java | 10 +++++++--- .../java/org/archive/modules/net/RobotstxtTest.java | 8 ++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/net/Robotstxt.java b/modules/src/main/java/org/archive/modules/net/Robotstxt.java index 40d6e7a8..c11bd34f 100644 --- a/modules/src/main/java/org/archive/modules/net/Robotstxt.java +++ b/modules/src/main/java/org/archive/modules/net/Robotstxt.java @@ -93,10 +93,14 @@ public class Robotstxt implements Serializable { String[] lines = LINE_SEPARATOR.split(buffer); if (buffer.limit() == buffer.capacity()) { - int processed = buffer.capacity() - lines[lines.length - 1].length(); + int processed = buffer.capacity(); + if (lines.length != 0) { + // discard the partial line at the end so we don't process a truncated path + int last = lines.length - 1; + processed -= lines[last].length(); + lines[last] = ""; + } logger.warning("processed " + processed + " characters, ignoring the rest (see HER-1990)"); - // discard the partial line at the end so we don't process a truncated path - lines[lines.length - 1] = ""; } // current is the disallowed paths for the preceding User-Agent(s) diff --git a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java index 559bd7d6..ec102294 100644 --- a/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java +++ b/modules/src/test/java/org/archive/modules/net/RobotstxtTest.java @@ -243,4 +243,12 @@ public class RobotstxtTest extends TestCase { assertTrue("and also ignore any lines after the size limit", rt.getDirectivesFor("c").allows("/foo")); } + + public void testAllBlankLines() throws IOException { + StringBuilder builder = new StringBuilder(); + for (int i = 0; i < Robotstxt.MAX_SIZE; i++) { + builder.append('\n'); + } + new Robotstxt(new StringReader(builder.toString())); + } }