[2428] Handle garbage after the include

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.

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",