diff --git a/src/lib/dns/master_lexer.cc b/src/lib/dns/master_lexer.cc index b3b78c0e68..e6e2c7867c 100644 --- a/src/lib/dns/master_lexer.cc +++ b/src/lib/dns/master_lexer.cc @@ -149,6 +149,11 @@ MasterLexer::popSource() { impl_->has_previous_ = false; } +size_t +MasterLexer::getSourceCount() const { + return (impl_->sources_.size()); +} + std::string MasterLexer::getSourceName() const { if (impl_->sources_.empty()) { diff --git a/src/lib/dns/master_lexer.h b/src/lib/dns/master_lexer.h index 99b433f8b5..cdf2866419 100644 --- a/src/lib/dns/master_lexer.h +++ b/src/lib/dns/master_lexer.h @@ -404,6 +404,11 @@ public: /// \throw isc::InvalidOperation Called with no pushed source. void popSource(); + /// \brief Get number of sources inside the lexer. + /// + /// This method never throws. + size_t getSourceCount() const; + /// \brief Return the name of the current input source name. /// /// If it's a file, it will be the C string given at the corresponding diff --git a/src/lib/dns/master_loader.cc b/src/lib/dns/master_loader.cc index 941a100948..0a3db399cb 100644 --- a/src/lib/dns/master_loader.cc +++ b/src/lib/dns/master_loader.cc @@ -22,13 +22,29 @@ #include #include +#include // for iequals using std::string; using std::auto_ptr; +using boost::algorithm::iequals; namespace isc { namespace dns { +namespace { + +// An internal exception, used to control the code flow in case of errors. +// It is thrown during the loading and caught later, not to be propagated +// outside of the file. +class InternalException : public isc::Exception { +public: + InternalException(const char* filename, size_t line, const char* what) : + Exception(filename, line, what) + {} +}; + +} // end unnamed namespace + class MasterLoader::MasterLoaderImpl { public: MasterLoaderImpl(const char* master_file, @@ -69,9 +85,7 @@ public: std::string error; if (!lexer_.pushSource(filename.c_str(), &error)) { if (initialized_) { - // $INCLUDE file - reportError(lexer_.getSourceName(), lexer_.getSourceLine(), - error); + isc_throw(InternalException, error.c_str()); } else { // Top-level file reportError("", 0, error); @@ -81,6 +95,14 @@ public: initialized_ = true; } + bool popSource() { + if (lexer_.getSourceCount() == 1) { + return (false); + } + lexer_.popSource(); + return (true); + } + void pushStreamSource(std::istream& stream) { lexer_.pushSource(stream); initialized_ = true; @@ -94,6 +116,74 @@ public: bool loadIncremental(size_t count_limit); + void doInclude() { + // First, get the filename to include + const string + filename(lexer_.getNextToken(MasterToken::QSTRING).getString()); + + // There could be an origin (or maybe not). So try looking + const MasterToken name_tok(lexer_.getNextToken(MasterToken::QSTRING, + true)); + + if (name_tok.getType() == MasterToken::QSTRING || + name_tok.getType() == MasterToken::STRING) { + // TODO: Handle the origin. Once we complete #2427. + } else { + // We return the newline there. This is because after we pop + // the source, we want to call eatUntilEOL and this would + // eat to the next one. + lexer_.ungetToken(); + } + + pushSource(filename); + } + + void handleDirective(const char* directive, size_t length) { + if (iequals(directive, "INCLUDE")) { + doInclude(); + } else if (iequals(directive, "ORIGIN")) { + // TODO: Implement + isc_throw(isc::NotImplemented, + "Origin directive not implemented yet"); + } else if (iequals(directive, "TTL")) { + // TODO: Implement + isc_throw(isc::NotImplemented, + "TTL directive not implemented yet"); + } else { + isc_throw(InternalException, "Unknown directive '" << + string(directive, directive + length) << "'"); + } + } + + 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(), + "File does not end with newline"); + // 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_; @@ -133,8 +223,15 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) { do { const MasterToken& empty_token(lexer_.getNextToken()); if (empty_token.getType() == MasterToken::END_OF_FILE) { - // TODO: Check if this is the last source, possibly pop - return (true); + if (!popSource()) { + return (true); + } else { + // We try to read a token from the popped source + // So retry the loop, but first, make sure the source + // is at EOL + eatUntilEOL(true); + continue; + } } empty = empty_token.getType() == MasterToken::END_OF_LINE; } while (empty); @@ -144,7 +241,19 @@ MasterLoader::MasterLoaderImpl::loadIncremental(size_t count_limit) { const MasterToken::StringRegion& name_string(lexer_.getNextToken(MasterToken::QSTRING). getStringRegion()); - // TODO $ handling + + if (name_string.len > 0 && name_string.beg[0] == '$') { + // This should have either thrown (and the error handler + // will read up until the end of line) or read until the + // end of line. + + // Exclude the $ from the string on this point. + handleDirective(name_string.beg + 1, name_string.len - 1); + // So, get to the next line, there's nothing more interesting + // in this one. + continue; + } + const Name name(name_string.beg, name_string.len, &zone_origin_); // TODO: Some more flexibility. We don't allow omitting @@ -199,27 +308,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(), - "File does not end with newline"); - // TODO: Try pop in case this is not the only - // source - return (true); - 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_lexer_unittest.cc b/src/lib/dns/tests/master_lexer_unittest.cc index b2415dab86..42ed3fb084 100644 --- a/src/lib/dns/tests/master_lexer_unittest.cc +++ b/src/lib/dns/tests/master_lexer_unittest.cc @@ -60,8 +60,10 @@ TEST_F(MasterLexerTest, preOpen) { } TEST_F(MasterLexerTest, pushStream) { + EXPECT_EQ(0, lexer.getSourceCount()); lexer.pushSource(ss); EXPECT_EQ(expected_stream_name, lexer.getSourceName()); + EXPECT_EQ(1, lexer.getSourceCount()); // From the point of view of this test, we only have to check (though // indirectly) getSourceLine calls InputSource::getCurrentLine. It should @@ -70,18 +72,22 @@ TEST_F(MasterLexerTest, pushStream) { // By popping it the stack will be empty again. lexer.popSource(); + EXPECT_EQ(0, lexer.getSourceCount()); checkEmptySource(lexer); } TEST_F(MasterLexerTest, pushFile) { // We use zone file (-like) data, but in this test that actually doesn't // matter. + EXPECT_EQ(0, lexer.getSourceCount()); EXPECT_TRUE(lexer.pushSource(TEST_DATA_SRCDIR "/masterload.txt")); + EXPECT_EQ(1, lexer.getSourceCount()); EXPECT_EQ(TEST_DATA_SRCDIR "/masterload.txt", lexer.getSourceName()); EXPECT_EQ(1, lexer.getSourceLine()); lexer.popSource(); checkEmptySource(lexer); + EXPECT_EQ(0, lexer.getSourceCount()); // If we give a non NULL string pointer, its content will be intact // if pushSource succeeds. diff --git a/src/lib/dns/tests/master_loader_unittest.cc b/src/lib/dns/tests/master_loader_unittest.cc index 70e32490fb..bfbf9fa357 100644 --- a/src/lib/dns/tests/master_loader_unittest.cc +++ b/src/lib/dns/tests/master_loader_unittest.cc @@ -115,6 +115,14 @@ public: compare(current->getRdataIterator()->getCurrent())); } + void checkBasicRRs() { + checkRR("example.org", RRType::SOA(), + "ns1.example.org. admin.example.org. " + "1234 3600 1800 2419200 7200"); + checkRR("example.org", RRType::NS(), "ns1.example.org."); + checkRR("www.example.org", RRType::A(), "192.0.2.1"); + } + MasterLoaderCallbacks callbacks_; boost::scoped_ptr loader_; vector errors_; @@ -135,11 +143,60 @@ TEST_F(MasterLoaderTest, basicLoad) { EXPECT_TRUE(errors_.empty()); EXPECT_TRUE(warnings_.empty()); - checkRR("example.org", RRType::SOA(), - "ns1.example.org. admin.example.org. " - "1234 3600 1800 2419200 7200"); - checkRR("example.org", RRType::NS(), "ns1.example.org."); - checkRR("www.example.org", RRType::A(), "192.0.2.1"); + checkBasicRRs(); +} + +// Test the $INCLUDE directive +TEST_F(MasterLoaderTest, include) { + // Test various cases of include + const char* includes[] = { + "$include", + "$INCLUDE", + "$Include", + "$InCluDe", + "\"$INCLUDE\"", + NULL + }; + for (const char** include = includes; *include != NULL; ++include) { + SCOPED_TRACE(*include); + + clear(); + // Prepare input source that has the include and some more data + // below (to see it returns back to the original source). + const string include_str = string(*include) + " " + + TEST_DATA_SRCDIR + "/example.org\nwww 3600 IN AAAA 2001:db8::1\n"; + stringstream ss(include_str); + setLoader(ss, Name("example.org."), RRClass::IN(), + MasterLoader::MANY_ERRORS); + + loader_->load(); + EXPECT_TRUE(loader_->loadedSucessfully()); + EXPECT_TRUE(errors_.empty()); + EXPECT_TRUE(warnings_.empty()); + + checkBasicRRs(); + checkRR("www.example.org", RRType::AAAA(), "2001:db8::1"); + } +} + +// Test the source is correctly popped even after error +TEST_F(MasterLoaderTest, popAfterError) { + const string include_str = "$include " TEST_DATA_SRCDIR + "/broken.zone\nwww 3600 IN AAAA 2001:db8::1\n"; + stringstream ss(include_str); + // We don't test without MANY_ERRORS, we want to see what happens + // after the error. + setLoader(ss, Name("example.org."), RRClass::IN(), + MasterLoader::MANY_ERRORS); + + loader_->load(); + EXPECT_FALSE(loader_->loadedSucessfully()); + EXPECT_EQ(1, errors_.size()); // For the broken RR + EXPECT_EQ(1, warnings_.size()); // For missing EOLN + + // The included file doesn't contain anything usable, but the + // line after the include should be there. + checkRR("www.example.org", RRType::AAAA(), "2001:db8::1"); } // Check it works the same when created based on a stream, not filename @@ -223,6 +280,15 @@ struct ErrorCase { { "www 3600 IN \"A\" 192.0.2.1", "Quoted type" }, { "unbalanced)paren 3600 IN A 192.0.2.1", "Token error 1" }, { "www 3600 unbalanced)paren A 192.0.2.1", "Token error 2" }, + // Check the unknown directive. The rest looks like ordinary RR, + // so we see the $ is actually special. + { "$UNKNOWN 3600 IN A 192.0.2.1", "Unknown $ directive" }, + { "$INCLUD " TEST_DATA_SRCDIR "/example.org", "Include too short" }, + { "$INCLUDES " TEST_DATA_SRCDIR "/example.org", "Include too long" }, + { "$INCLUDE", "Missing include path" }, + { "$INCLUDE /file/not/found", "Include file not found" }, + { "$INCLUDE /file/not/found and here goes bunch of garbage", + "Include file not found and garbage at the end of line" }, { NULL, NULL } }; @@ -242,7 +308,7 @@ TEST_F(MasterLoaderTest, brokenZone) { EXPECT_FALSE(loader_->loadedSucessfully()); EXPECT_THROW(loader_->load(), MasterLoaderError); EXPECT_FALSE(loader_->loadedSucessfully()); - EXPECT_EQ(1, errors_.size()) << errors_[0]; + EXPECT_EQ(1, errors_.size()); EXPECT_TRUE(warnings_.empty()); checkRR("example.org", RRType::SOA(), "ns1.example.org. " @@ -273,15 +339,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. " @@ -291,6 +357,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", diff --git a/src/lib/dns/tests/testdata/Makefile.am b/src/lib/dns/tests/testdata/Makefile.am index d312e07955..9494117f8d 100644 --- a/src/lib/dns/tests/testdata/Makefile.am +++ b/src/lib/dns/tests/testdata/Makefile.am @@ -171,6 +171,7 @@ EXTRA_DIST += tsig_verify4.spec tsig_verify5.spec tsig_verify6.spec EXTRA_DIST += tsig_verify7.spec tsig_verify8.spec tsig_verify9.spec EXTRA_DIST += tsig_verify10.spec EXTRA_DIST += example.org +EXTRA_DIST += broken.zone .spec.wire: $(PYTHON) $(top_builddir)/src/lib/util/python/gen_wiredata.py -o $@ $< diff --git a/src/lib/dns/tests/testdata/broken.zone b/src/lib/dns/tests/testdata/broken.zone new file mode 100644 index 0000000000..70f4540c1a --- /dev/null +++ b/src/lib/dns/tests/testdata/broken.zone @@ -0,0 +1,3 @@ +; This should fail due to broken TTL +; The file should _NOT_ end with EOLN +broken. 3600X IN A 192.0.2.2 More data \ No newline at end of file