GeoIP_database_info is not returning full string from database

[pbiering]

See also: https://bugzilla.redhat.com/show_bug.cgi?id=1426853

Looks like https://github.com/maxmind/geoip-api-c/issues/79 is not really fixed and also it looks like output depends on database version:

Old DBs from 2013 using library 1.6.9 (OK)

./test_geoip /tmp/geoip/Geo*.dat
INFO  : GeoIP library version: >1.6.9<
INFO  : file=/tmp/geoip/GeoIPASNum.dat                          GeoIP_database_info=>GEO-117 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIPASNumv6.dat                        GeoIP_database_info=>GEO-117 20130306 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIP.dat                               GeoIP_database_info=>GEO-106FREE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIPv6.dat                             GeoIP_database_info=>GEO-106FREE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoLiteCity.dat                         GeoIP_database_info=>GEO-533LITE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoLiteCityv6.dat                       GeoIP_database_info=>GEO-536LITE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<

Old DBs using library version 1.5.0 (OK)

/tmp/test_geoip /tmp/geoip/*.dat
INFO  : GeoIP library version: >1.5.0<
INFO  : file=/tmp/geoip/GeoIPASNum.dat                          GeoIP_database_info=>GEO-117 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIPASNumv6.dat                        GeoIP_database_info=>GEO-117 20130306 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIP.dat                               GeoIP_database_info=>GEO-106FREE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoIPv6.dat                             GeoIP_database_info=>GEO-106FREE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoLiteCity.dat                         GeoIP_database_info=>GEO-533LITE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<
INFO  : file=/tmp/geoip/GeoLiteCityv6.dat                       GeoIP_database_info=>GEO-536LITE 20131105 Build 1 Copyright (c) 2013 MaxMind Inc All Rights Reserved<

New DBs from 2016+ using library version 1.6.9 (BUGGY)

./test_geoip /var/local/share/GeoIP/*.dat
INFO  : GeoIP library version: >1.6.9<
INFO  : file=/var/local/share/GeoIP/GeoIPASNum.dat              GeoIP_database_info=>GEO-117 20161002 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoIPASNumv6.dat            GeoIP_database_info=>GEO-117 20160911 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Re<
INFO  : file=/var/local/share/GeoIP/GeoIPCity.dat               GeoIP_database_info=>GEO-533LITE 20161004 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoIPCityv6.dat             GeoIP_database_info=>GEO-536LITE 20161004 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reserved<
INFO  : file=/var/local/share/GeoIP/GeoIP.dat                   GeoIP_database_info=>GEO-106FREE 20161004 Build 1 Copyright (c) 2016 MaxMind <
INFO  : file=/var/local/share/GeoIP/GeoIPv6.dat                 GeoIP_database_info=>GEO-106FREE 20161004 Build 1 Copy<
INFO  : file=/var/local/share/GeoIP/GeoLiteCity.dat             GeoIP_database_info=>GEO-533LITE 20161004 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoLiteCityv6.dat           GeoIP_database_info=>GEO-536LITE 20161004 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reserved<
INFO  : file=/var/local/share/GeoIP/GeoLiteCountry.dat          GeoIP_database_info=>GEO-106FREE 20170103 Build 1 Copyright (c) 2017 MaxMind <

New DBs from 2016+ using library version 1.5.0 (BUGGY)

/tmp/test_geoip /var/local/share/GeoIP/*.dat
INFO  : GeoIP library version: >1.5.0<
INFO  : file=/var/local/share/GeoIP/GeoIPASNum.dat              GeoIP_database_info=>GEO-117 20170204 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoIPASNumv6.dat            GeoIP_database_info=>GEO-117 20170204 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Re<
INFO  : file=/var/local/share/GeoIP/GeoIPCity.dat               GeoIP_database_info=>GEO-533LITE 20170207 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoIPCityv6.dat             GeoIP_database_info=>GEO-536LITE 20170207 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Reserved<
INFO  : file=/var/local/share/GeoIP/GeoIP.dat                   GeoIP_database_info=>GEO-106FREE 20170207 Build 1 Copyright (c) 2017 MaxMind <
INFO  : file=/var/local/share/GeoIP/GeoIP-initial.dat           GeoIP_database_info=>GEO-106FREE 20160607 Build 1 Copyright (c) 2016 MaxMind <
INFO  : file=/var/local/share/GeoIP/GeoIPv6.dat                 GeoIP_database_info=>GEO-106FREE 20170207 Build 1 Copy<
INFO  : file=/var/local/share/GeoIP/GeoIPv6-initial.dat         GeoIP_database_info=>GEO-106FREE 20160607 Build 1 Copy<
INFO  : file=/var/local/share/GeoIP/GeoLiteCity.dat             GeoIP_database_info=>GEO-533LITE 20170207 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoLiteCityv6.dat           GeoIP_database_info=>GEO-536LITE 20170207 Build 1 Copyright (c) 2017 MaxMind Inc All Rights Reserved<

Test code is:

/*
 * compile with gcc -l GeoIP test_geoip.c -o test_geoi
 */
#include "GeoIP.h"
#include "GeoIPCity.h"
#include <stdio.h>

int main(int argc, char *argv[]) {
    if (argc <= 1) {
        printf("ERROR : no file given\n");
        exit(1);
    };

    GeoIP *gi = NULL;
    char *filename;
    int i;

    printf("INFO  : GeoIP library version: >%s<\n", GeoIP_lib_version());

    for (i = 1; i < argc; i++) {
        filename = argv[i];
        gi = GeoIP_open(filename, GEOIP_STANDARD);
        if (gi != NULL) {
            printf("INFO  : file=%-50s GeoIP_database_info=>%s<\n", filename, GeoIP_database_info(gi));
            GeoIP_delete(gi);
        } else {
            printf("ERROR : file=%-50s can't opened\n", filename);
        };
    };
}

[oschwald]

The fix in #80 only fixed some breakage introduced in #74 where the offset calculation was unintentionally changed. Based on your description, it sounds like what you are seeing predates that issue. Do you happen to know if ay version correctly parses the version strings? The relevant code is here.

[pbiering]

For "ipv6calc" compatibility tests I have here all older versions and it looks like that current binary dat files are incompatible at least since version 1.4.4:

LD_LIBRARY_PATH=~/tmp/GeoIP-1.4.4/libGeoIP/.libs/ ./test_geoip_no_version /var/local/share/GeoIP/*.dat
INFO  : file=/var/local/share/GeoIP/GeoIPASNum.dat              GeoIP_database_info=>GEO-117 20161002 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Reser<
INFO  : file=/var/local/share/GeoIP/GeoIPASNumv6.dat            GeoIP_database_info=>GEO-117 20160911 Build 1 Copyright (c) 2016 MaxMind Inc All Rights Re<

Imho one has to check/track changes in the dat file generator by code review and/or testing older releases of the dat file to see when this bug was introduced.

[pghmcfc]

It looks to me that the current code finds the start of the database info string successfully, but is prone to truncating the string. Without knowing what the legacy database format looks like, it's hard to debug this in my head but it seems a bit strange that the loop variable i is involved both in finding the start of the string and its length. Would it not work to keep the code that finds the start of the database info and then take everything from there to a NULL character as long as it doesn't overflow the malloc-ed buffer?

[oschwald]

That might work. I am not sure why the code was designed that way. This is further complicated by the fact that there are several different variations of the legacy format, all without detailed specifications.

[pghmcfc]

I've just been looking at this and I think the current code is correct. The problem is the databases, not the code. The database info structure is at the end of the file, and by dumping the file contents using od, it's clear that the current GeoLite legacy databases have truncated database info strings, matching the output from @pbiering's test program. So it's the database generator that needs looking at.

[oschwald]

@pghmcfc, thanks for looking into it! We'll take a look at the generation code.

[klp2]

This should be fixed in the latest GeoLite legacy databases.

[oschwald]

I am going to close this issue as both the database reader and generation should now be fixed. Thanks for reporting!

[pghmcfc]

The new databases look good to me. Thanks!