[tor-bugs] #25515 [Core Tor/Tor]: Add a unit test for geoip_load_file()

[Tor Bug Tracker & Wiki]

#25515: Add a unit test for geoip_load_file()
--------------------------+------------------------------------
 Reporter:  nickm         |          Owner:  nickm
     Type:  enhancement   |         Status:  needs_review
 Priority:  Medium        |      Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  isis          |        Sponsor:
--------------------------+------------------------------------

Comment (by isis):

 Couple trivial things:

  1. We can remove the "is this necessary?" comments on things that are
 tested twice between `test_geoip_load_file()` and
 `test_geoip6_load_file()`. I think it's fine to test the same thing more
 than once; it's better than not testing at all, or assuming another test
 will catch the bad behaviour you have in mind.
  2. couple typos: s/1nd/1st/ s/2st/2nd/
  3. I think we might need `TT_FORK|SKIP_ON_WINDOWS` for
 `test_geoip6_load_file()`.
  4. Same as !#3, but also for `test_geoip_load_2nd_file()`.

 I've applied your patches to a `bug25515`
 [https://gitweb.torproject.org/user/isis/tor.git/log/?h=bug25515 branch]
 based on the latest master, and [https://travis-
 ci.org/isislovecruft/tor/builds/367311635 the Travis tests fail] because
 of the unused variables `num_countries_geoip2` and `num_countries_geoip`.
 Since the patches didn't automatically apply for some reason, I went ahead
 and added some extra commits fixing the above nitpicks while I was
 manually applying.

 -----

 Less trivial issues:

  1. In your `test_geoip_load_2nd_file()` test, there's a FIXME:

  {{{#!c
   int num_countries_geoip = geoip_get_n_countries();

   /* Load 2st geoip (empty) file */
   const char FNAME2[] = SRCDIR "/src/test/geoip_dummy";
   tt_int_op(0, OP_EQ, geoip_load_file(AF_INET, FNAME2));

   int num_countries_geoip2 = geoip_get_n_countries();
   /* FIXME: should not this be different? */
   /* tt_int_op(num_countries_geoip, OP_NE, num_countries_geoip2); */
  }}}

  The relevant bits of code from `geoip_load_file()` are:

  {{{#!c
 int
 geoip_load_file(sa_family_t family, const char *filename)
 {
   FILE *f;
   const char *msg = "";
   const or_options_t *options = get_options();
   int severity = options_need_geoip_info(options, &msg) ? LOG_WARN :
 LOG_INFO;
   crypto_digest_t *geoip_digest_env = NULL;

   tor_assert(family == AF_INET || family == AF_INET6);

   if (!(f = tor_fopen_cloexec(filename, "r"))) {
     log_fn(severity, LD_GENERAL, "Failed to open GEOIP file %s.  %s",
            filename, msg);
     return -1;
   }
   if (!geoip_countries)
     init_geoip_countries();

   if (family == AF_INET) {
     if (geoip_ipv4_entries) {
       SMARTLIST_FOREACH(geoip_ipv4_entries, geoip_ipv4_entry_t *, e,
                         tor_free(e));
       smartlist_free(geoip_ipv4_entries);
     }
     geoip_ipv4_entries = smartlist_new();
   } else { /* AF_INET6 */
     // [snip]
   }
   geoip_digest_env = crypto_digest_new();

   log_notice(LD_GENERAL, "Parsing GEOIP %s file %s.",
              (family == AF_INET) ? "IPv4" : "IPv6", filename);
   while (!feof(f)) {
     char buf[512];
     if (fgets(buf, (int)sizeof(buf), f) == NULL)
       break;
     crypto_digest_add_bytes(geoip_digest_env, buf, strlen(buf));
     /* FFFF track full country name. */
     geoip_parse_entry(buf, family);
   }
   /*XXXX abort and return -1 if no entries/illformed?*/
   fclose(f);
  }}}

  So it's clearing all the ''entries'' (but curiously ''not'' the
 countries!), which is quite non-intuitive and doesn't appear to be
 documented anywhere, so I'm not even sure if this is the intended
 behaviour. Then you're hitting the `fclose()` there since the file is
 empty, so nothing else happens.

 I'm not sure what to do about the non-intuitive and possibly unintentional
 behaviour, i.e. whether we should also be clearing the countries in
 `geoip_load_file()` when we clear the entries? I think we actually want to
 be calling `clear_geoip_db()` in `geoip_load_file()`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25515#comment:26>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online