use clock() as source for SRV randomness

Initializing a random number generator with the time since epoch could
be good enough, but reaches its limits in test code as the 100
iterations might very well happen in the same second and hence the seed
number is always the same… clock() has a way lower resolution so it
changes more often and not unimportant: If many users start the update
at the same time it isn't to unlikely the SRV record will be ordered in
the same second choosing the same for them all, but it seems less likely
that the exact same clock() time has passed for them.

And if I have to touch this, lets change a few other things as well to
make me and/or compilers a bit happier (clang complained about the usage
of a GNU extension in the testcase for example).

diff --git a/apt-pkg/contrib/srvrec.cc b/apt-pkg/contrib/srvrec.cc
index b4a3d97d25d9abe0a04781967a5807f4a02ea290..1746682743e8d14b1a285f6944526aa730defe71 100644 (file)
--- a/apt-pkg/contrib/srvrec.cc
+++ b/apt-pkg/contrib/srvrec.cc
@@ -13,7 +13,7 @@
 #include <netinet/in.h>
 #include <arpa/nameser.h>
 #include <resolv.h>
-#include <chrono>
+#include <time.h>
 
 #include <algorithm>
 
@@ -68,7 +68,6 @@ bool GetSrvRecords(std::string name, std::vector<SrvRec> &Result)
    unsigned char *pt = answer+sizeof(HEADER)+compressed_name_len+QFIXEDSZ;
    while ((int)Result.size() < answer_count && pt < answer+answer_len)
    {
-      SrvRec rec;
       u_int16_t type, klass, priority, weight, port, dlen;
       char buf[MAXDNAME];
 
@@ -105,11 +104,7 @@ bool GetSrvRecords(std::string name, std::vector<SrvRec> &Result)
       pt += compressed_name_len;
 
       // add it to our class
-      rec.priority = priority;
-      rec.weight = weight;
-      rec.port = port;
-      rec.target = buf;
-      Result.push_back(rec);
+      Result.emplace_back(buf, priority, weight, port);
    }
 
    // implement load balancing as specified in RFC-2782
@@ -173,21 +168,14 @@ SrvRec PopFromSrvRecs(std::vector<SrvRec> &Recs)
 #endif
 
    // shuffle in a very simplistic way for now (equal weights)
-   std::vector<SrvRec>::iterator I, J;
-   I = J = Recs.begin();
-   for(;I != Recs.end(); ++I)
-   {
-      if(I->priority != J->priority)
-         break;
-   }
-
-   // FIXME: meeeeh, where to init this properly
-   unsigned seed = std::chrono::system_clock::now().time_since_epoch().count();
-   std::shuffle(J, I, std::default_random_engine(seed));
-
-   // meh, no pop_front() in std::vector?
-   SrvRec selected = *Recs.begin();
-   Recs.erase(Recs.begin());
+   std::vector<SrvRec>::const_iterator I = Recs.begin();
+   std::vector<SrvRec>::const_iterator const J = std::find_if(Recs.cbegin(), Recs.cend(),
+        [&I](SrvRec const &J) { return I->priority != J.priority; });
+
+   // clock seems random enough.
+   I += clock() % std::distance(I, J);
+   SrvRec const selected = std::move(*I);
+   Recs.erase(I);
 
    if (_config->FindB("Debug::Acquire::SrvRecs", false) == true)
       std::cerr << "PopFromSrvRecs: selecting " << selected.target << std::endl;

diff --git a/apt-pkg/contrib/srvrec.h b/apt-pkg/contrib/srvrec.h
index e07edc68322377fde5389d25c96021eb15105773..2adad03e9e064383c0a2473de6e6386c9a7ad137 100644 (file)
--- a/apt-pkg/contrib/srvrec.h
+++ b/apt-pkg/contrib/srvrec.h
@@ -26,9 +26,15 @@ class SrvRec
    int random_number_range_end;
    int random_number_range_max;
 
-   bool operator<(SrvRec const &other) const { 
-      return this->priority < other.priority; 
+   bool operator<(SrvRec const &other) const {
+      return this->priority < other.priority;
    }
+
+   SrvRec(std::string const Target, u_int16_t const Priority,
+        u_int16_t const Weight, u_int16_t const Port) :
+      target(Target), priority(Priority), weight(Weight), port(Port),
+      random_number_range_start(0), random_number_range_end(0),
+      random_number_range_max(0) {}
 };
 
 /** \brief Get SRV records from host/port (builds the query string internally) 

diff --git a/cmdline/apt-helper.cc b/cmdline/apt-helper.cc
index 3c49bf1492fe9e8a8c5a8275d2285c2ab4f2d479..2d24a8aee8b351cad0cf6317e76b556b05268a13 100644 (file)
--- a/cmdline/apt-helper.cc
+++ b/cmdline/apt-helper.cc
@@ -82,25 +82,27 @@ static bool DoDownloadFile(CommandLine &CmdL)
 static bool DoSrvLookup(CommandLine &CmdL)
 {
    if (CmdL.FileSize() < 1)
-      return _error->Error(_("Must specifc at least one srv record"));
+      return _error->Error("Must specify at least one SRV record");
 
-   std::vector<SrvRec> srv_records;
-   c1out << "# target priority weight port" << std::endl;
-   for(int i=1; CmdL.FileList[i] != NULL; i++)
+   for(size_t i = 1; CmdL.FileList[i] != NULL; ++i)
    {
-      if(GetSrvRecords(CmdL.FileList[i], srv_records) == false)
-         _error->Warning(_("GetSrvRec failed for %s"), CmdL.FileList[i]);
-      for (std::vector<SrvRec>::const_iterator I = srv_records.begin();
-           I != srv_records.end(); ++I)
+      std::vector<SrvRec> srv_records;
+      std::string const name = CmdL.FileList[i];
+      c0out << "# Target\tPriority\tWeight\tPort # for " << name << std::endl;
+      size_t const found = name.find(":");
+      if (found != std::string::npos)
       {
-         c1out << (*I).target.c_str() << " "
-               << (*I).priority << " "
-               << (*I).weight << " "
-               << (*I).port << " "
-               << std::endl;
+        std::string const host = name.substr(0, found);
+        size_t const port = atoi(name.c_str() + found + 1);
+        if(GetSrvRecords(host, port, srv_records) == false)
+           _error->Warning(_("GetSrvRec failed for %s"), name.c_str());
       }
-   }
+      else if(GetSrvRecords(name, srv_records) == false)
+        _error->Warning(_("GetSrvRec failed for %s"), name.c_str());
 
+      for (SrvRec const &I : srv_records)
+        c1out << I.target << "\t" << I.priority << "\t" << I.weight << "\t" << I.port << std::endl;
+   }
    return true;
 }
 

diff --git a/test/libapt/srvrecs_test.cc b/test/libapt/srvrecs_test.cc
index 7e43cc7572249313010bf44f3d7fee96e55c84a2..4b63d2ccd08e8e801246488fba79bd06af156d95 100644 (file)
--- a/test/libapt/srvrecs_test.cc
+++ b/test/libapt/srvrecs_test.cc
@@ -12,20 +12,28 @@ TEST(SrvRecTest, PopFromSrvRecs)
    // the PopFromSrvRecs() is using a random number so we must
    // run it a bunch of times to ensure we are not fooled by randomness
    std::set<std::string> selected;
-   for(int i=0;i<100;i++)
+   for(size_t i = 0; i < 100; ++i)
    {
       std::vector<SrvRec> Meep;
-      SrvRec foo = {target:"foo", priority: 20, weight: 0, port: 80};
-      Meep.push_back(foo);
-      
-      SrvRec bar = {target:"bar", priority: 20, weight: 0, port: 80};
-      Meep.push_back(bar);
+      Meep.emplace_back("foo", 20, 0, 80);
+      Meep.emplace_back("bar", 20, 0, 80);
+      Meep.emplace_back("baz", 30, 0, 80);
 
-      EXPECT_EQ(Meep.size(), 2);
-      SrvRec result = PopFromSrvRecs(Meep);
+      EXPECT_EQ(Meep.size(), 3);
+      SrvRec const result = PopFromSrvRecs(Meep);
       selected.insert(result.target);
       // ensure that pop removed one element
+      EXPECT_EQ(Meep.size(), 2);
+      EXPECT_NE(result.target, "baz");
+
+      SrvRec const result2 = PopFromSrvRecs(Meep);
+      EXPECT_NE(result.target, result2.target);
+      EXPECT_NE(result2.target, "baz");
       EXPECT_EQ(Meep.size(), 1);
+
+      SrvRec const result3 = PopFromSrvRecs(Meep);
+      EXPECT_EQ(result3.target, "baz");
+      EXPECT_TRUE(Meep.empty());
    }
 
    // ensure that after enough runs we end up with both selected