OGRFeature does not always check the provided field index

[Ari Jolma]

For example the IsFieldSet method causes a segfault if the provided iField is invalid. In a perfect world the method should return one of three: field is set, field is not set, field does not exist in the feature. IMO the latter two can be combined as the third is in fact a subcase of the second.

A feature has always a valid feature definition object, which has field count. Feature methods can thus always ask from the definition object the field count and this should be included in methods like IsFieldSet.

[Ari Jolma]

Hm, I just realized that in the case (​http://lists.osgeo.org/pipermail/gdal-dev/2010-April/024418.html), which led to me proposing this solution (which I think is still valid for some cases), the underlying definition object changes. However, the underlying definition object knows if there are existing features based on it (the ref count). Thus, we could consider disallowing adding new fields while ref count is not zero.

[Even Rouault]

r19557 /trunk/gdal/ogr/ogrfeature.cpp: Add validation of field index in OGR_F_IsFieldSet() (related to #3552) --> but doesn't fix the issue from the original reporter

Your idea of checking the ref count of the feature definition makes sense (it would be against 1 and not instead 0 as when the layer defn is created the ref count is incremented to 1). I had also thought about it. However from C++, the user can increment/decrement the ref count at will with the OGRFeatureDefn::Reference() / Dereference() methods, so the ref count can be > 1 but there could still no feature using it. This is probably mostly a theoretical case as I doubt anybody uses this. Another difficulty is that there's no centralized way of introducing the check. All drivers should be updated to add the check in their CreateField() methods. I had though of adding it in OGRFeatureDefn::AddFieldDefn(), but all drivers rely on the fact that this method cannot fail, so the check must be done before.

Perhaps the solution is to put the check in the C method OGR_L_CreateField() to avoid Perl/Python/Java users to make errors (too bad for C++ users, but they have many other ways of crashing their app by misusing the API)

[Ari Jolma]

I support adding the refcount check into OGR_L_CreateField to make sure that invalid feature objects do not exist.

[Even Rouault]

I discovered that the proposed method wouldn't work for the OGR MEM driver. It maintains an internal collection of features refcounting the layer defn, and manages properly their update when CreateField() is called.

[Jukka Rahkonen]

Is the status of this ticket perhaps still "partly fixed"?

[Ari Jolma]

I'm closing this. Reopen if this becomes a real problem.