GeoIP_db_avail() doesn't distinguish between revisions

[each]

GeoIP_db_avail() checks whether a file for a database exists, but doesn't check
the database type to see whether the file contains the requested edition.

I had code that tested whether GEOIP_REGION_EDITION_REV1 was available,
and if not, it would try GEOIP_REGION_EDITION_REV0. It would then open
whichever one turned out to be available. I had assumed, until I looked at the
code, that since the argument passed to GeoIP_db_avail() was a database edition
code, that it would be checking to see whether that particular edition was
supported or not. However, since both editions use the same filename,
GeoIP_db_avail() reported that REV1 was available, even when it was REV0
that was installed.

In libGeoIP 1.5.0, a type check was added in Geoip_open_type(), so that it
now returns NULL if the database doesn't have the expected type value.
The result is that GeoIP_open_type() can fail even if GeoIP_db_avail() just
returned true for the exact same database type, which is kind of unexpected.

I've changed my code so it no longer relies on GeoIP_db_avail(), but this still seems
like problematic behavior and I thought I'd better mention it. I can submit a patch, if it would be useful.

[borisz]

Hi Evan,

it is like you said, GeoIP_db_avail did not know the type. It is more like a file test.
You could use GeoIP_open_type instead of GeoIP_db_avail, to make sure the database type is really available.
Or use gi = GeoIP_open and get the database type from gi->databaseType.
Nowadays this behavior hit almost only GEOIP_CITY_EDITION_REV0 and GEOIP_CITY_EDITION_REV1 users b/c the content is a bit different.
AFAIK GEOIP_REGION_EDITION_REV0’s last update was 2003.

Boris Zentner

Am 23.01.2014 um 20:01 schrieb Evan Hunt notifications@github.com:

GeoIP_db_avail() checks whether a file for a database exists, but doesn't check
the database type to see whether the file contains the requested edition.

I had code that tested whether GEOIP_REGION_EDITION_REV1 was available,
and if not, it would try GEOIP_REGION_EDITION_REV0. It would then open
whichever one turned out to be available. I had assumed, until I looked at the
code, that since the argument passed to GeoIP_db_avail() was a database edition
code, that it would be checking to see whether that particular edition was
supported or not. However, since both editions use the same filename,
GeoIP_db_avail() reported that REV1 was available, even when it was REV0
that was installed.

In libGeoIP 1.5.0, a type check was added in Geoip_open_type(), so that it
now returns NULL if the database doesn't have the expected type value.
The result is that GeoIP_open_type() can fail even if GeoIP_db_avail() just
returned true for the exact same database type, which is kind of unexpected.

I've changed my code so it no longer relies on GeoIP_db_avail(), but this still seems
like problematic behavior and I thought I'd better mention it. I can submit a patch, if it would be useful.

—
Reply to this email directly or view it on GitHub.

[each]

On Thu, Jan 23, 2014 at 01:03:29PM -0800, Boris Zentner wrote:

it is like you said, GeoIP_db_avail did not know the type. It is
more like a file test.

Yes, I understand where I went wrong. But I do think it would be
better if GeoIP_db_avail() checked the databaseType value before
responding that a given edition was available -- at least in cases
where there's potential ambiguity because the same file name is
used by more than one type. I'd be happy to submit code for this.

If you don't think it would be an appropriate change, though, it
still might be helpful to add a comment in GeoIP.h explaining the
limitation, something like:

/*

• WARNING: GeoIP_db_avail() checks for the existence of a database
• file but does not check that it has the requested database revision.
• For database types which have more than one revision (including Region,
• City, and Cityv6), this can lead to unexpected results. Check the
• return value of GeoIP_open_type() to find out whether a particular
• database type is really available.
  */
  GEOIP_API int GeoIP_db_avail(int type);

You could use GeoIP_open_type instead of GeoIP_db_avail, to make
sure the database type is really available.

In fact that's exactly what I ended up doing. :)

[borisz]

Hi Evan,

Am 23.01.2014 um 22:46 schrieb Evan Hunt notifications@github.com:

On Thu, Jan 23, 2014 at 01:03:29PM -0800, Boris Zentner wrote:

it is like you said, GeoIP_db_avail did not know the type. It is
more like a file test.

Yes, I understand where I went wrong. But I do think it would be
better if GeoIP_db_avail() checked the databaseType value before
responding that a given edition was available -- at least in cases
where there's potential ambiguity because the same file name is
used by more than one type. I'd be happy to submit code for this.

I think we should just keep it as it is. For the following reasons:

• Backward compatibility.
• Searching for the real type is more expensive than the file test.
• You have to check the return code of open* anyway
• In most(all?) cases we could use GeoIP_open_type, no need to use GeoIP_db_avail
• The new GeoIP_db_avail function could be as easy as the (untested) code below. But GeoIP_db_avail is superfluous for all my use cases. I always ended up in using GeoIP_open_type or GeoIP_open.

int GeoIP_db_really_avail(int type) {
GeoIP* gi = GeoIP_open_type(type, GEOIP_STANDARD);
if (gi)
GeoIP_delete(gi);
return !!gi;
}

If you don't think it would be an appropriate change, though, it
still might be helpful to add a comment in GeoIP.h explaining the
limitation, something like:

/*

• WARNING: GeoIP_db_avail() checks for the existence of a database
• file but does not check that it has the requested database revision.
• For database types which have more than one revision (including Region,
• City, and Cityv6), this can lead to unexpected results. Check the
• return value of GeoIP_open_type() to find out whether a particular
• database type is really available.
  */

That sound like a great idea, I’ll copy your comment.

GEOIP_API int GeoIP_db_avail(int type);

You could use GeoIP_open_type instead of GeoIP_db_avail, to make
sure the database type is really available.

In fact that's exactly what I ended up doing. :)

Same for me.

—
Reply to this email directly or view it on GitHub.

[oschwald]

Closing this as we included the comment and we are not likely to make significant changes to this legacy API.