From aae73017c63bcb2527bf36a3abb2c436072e3a98 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 11 Dec 2012 19:49:24 +0100 Subject: [PATCH] [2428] Handle garbage after the include --- src/lib/dns/master_loader.cc | 69 ++++++++++++--------- src/lib/dns/tests/master_loader_unittest.cc | 26 +++++++- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc index 40710c29a7..e73ccee615 100644 --- a/src/lib/dns/master_loader.cc +++ b/src/lib/dns/master_loader.cc @@ -93,8 +93,11 @@ public: } bool popSource() { + if (--source_count_ == 0) { + return (false); + } lexer_.popSource(); - return (--source_count_ != 0); + return (true); } void pushStreamSource(std::istream& stream) { @@ -117,17 +120,12 @@ public: filename(lexer_.getNextToken(MasterToken::QSTRING). getStringRegion()); - // TODO: Handle the case where there's Name after the - // filename, meaning origin. Once $ORIGIN handling is - // done, it should be interconnected somehow. + // TODO: Handle Origin // Push the filename. We abuse the fact that filename // may not contain '\0' anywhere in it, so we can // freely use the filename.beg directly. pushSource(filename.beg); - - // TODO: Eat any extra tokens at the end of line (they - // should not be here, of course). } void handleDirective(const char* directive, size_t length) { @@ -152,6 +150,35 @@ public: } } + void eatUntilEOL(bool reportExtra) { + // We want to continue. Try to read until the end of line + for (;;) { + const MasterToken& token(lexer_.getNextToken()); + switch (token.getType()) { + case MasterToken::END_OF_FILE: + callbacks_.warning(lexer_.getSourceName(), + lexer_.getSourceLine(), + "Unexpected end ond of file"); + // We don't pop here. The End of file will stay there, + // and we'll handle it in the next iteration of + // loadIncremental properly. + return; + case MasterToken::END_OF_LINE: + // Found the end of the line. Good. + return; + default: + // Some other type of token. + if (reportExtra) { + reportExtra = false; + reportError(lexer_.getSourceName(), + lexer_.getSourceLine(), + "Extra tokens at the end of line"); + } + break; + } + } + } + private: MasterLexer lexer_; const Name zone_origin_; @@ -196,7 +223,9 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) { return (true); } else { // We try to read a token from the popped source - // So retry the loop + // So retry the loop, but first, make sure the source + // is at EOL + eatUntilEOL(true); continue; } } @@ -274,29 +303,7 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) { // propagated. reportError(lexer_.getSourceName(), lexer_.getSourceLine(), e.what()); - // We want to continue. Try to read until the end of line - bool end = false; - do { - const MasterToken& token(lexer_.getNextToken()); - switch (token.getType()) { - case MasterToken::END_OF_FILE: - callbacks_.warning(lexer_.getSourceName(), - lexer_.getSourceLine(), - "Unexpected end ond of file"); - if (!popSource()) { - return (true); - } - // Else: fall through, the popped source is - // at the end of line currently - case MasterToken::END_OF_LINE: - end = true; - break; - default: - // Do nothing. This is just to make compiler - // happy - break; - } - } while (!end); + eatUntilEOL(false); } } // When there was a fatal error and ok is false, we say we are done. diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc index e79e2f8ca7..73188d9cf8 100644 --- a/src/lib/dns/tests/master_loader_unittest.cc +++ b/src/lib/dns/tests/master_loader_unittest.cc @@ -336,15 +336,15 @@ TEST_F(MasterLoaderTest, brokenZone) { { SCOPED_TRACE("Error at EOF"); // This case is interesting only in the lenient mode. - const string zoneEOF(prepareZone(ec->line, false)); clear(); + const string zoneEOF(prepareZone(ec->line, false)); stringstream zone_stream(zoneEOF); setLoader(zone_stream, Name("example.org."), RRClass::IN(), MasterLoader::MANY_ERRORS); EXPECT_FALSE(loader_->loadedSucessfully()); EXPECT_NO_THROW(loader_->load()); EXPECT_FALSE(loader_->loadedSucessfully()); - EXPECT_EQ(1, errors_.size()); + EXPECT_EQ(1, errors_.size()) << errors_[0] << "\n" << errors_[1]; // The unexpected EOF warning EXPECT_EQ(1, warnings_.size()); checkRR("example.org", RRType::SOA(), "ns1.example.org. " @@ -354,6 +354,28 @@ TEST_F(MasterLoaderTest, brokenZone) { } } +// Check that a garbage after the include generates an error, but not fatal +// one (in lenient mode) and we can recover. +TEST_F(MasterLoaderTest, includeWithGarbage) { + // Include an origin (example.org) because we expect it to be handled + // soon and we don't want it to break here. + const string include_str("$INCLUDE " TEST_DATA_SRCDIR + "/example.org example.org bunch of other stuff\n" + "www 3600 IN AAAA 2001:db8::1\n"); + stringstream zone_stream(include_str); + setLoader(zone_stream, Name("example.org."), RRClass::IN(), + MasterLoader::MANY_ERRORS); + + EXPECT_NO_THROW(loader_->load()); + EXPECT_FALSE(loader_->loadedSucessfully()); + ASSERT_EQ(1, errors_.size()); + // It says something about extra tokens at the end + EXPECT_NE(string::npos, errors_[0].find("Extra")); + EXPECT_TRUE(warnings_.empty()); + checkBasicRRs(); + checkRR("www.example.org", RRType::AAAA(), "2001:db8::1"); +} + // Test the constructor rejects empty add callback. TEST_F(MasterLoaderTest, emptyCallback) { EXPECT_THROW(MasterLoader(TEST_DATA_SRCDIR "/example.org",