Opened 9 years ago

Closed 5 years ago

#3552 closed defect (worksforme)

OGRFeature does not always check the provided field index

Reported by: Ari Jolma Owned by: warmerdam
Priority: normal Milestone:
Component: OGR_SF Version: unspecified
Severity: normal Keywords:
Cc: even.rouault@…

Description

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?.

Change History (6)

comment:1 Changed 9 years ago by Ari Jolma

Cc: even.rouault@… added

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.

comment:2 Changed 9 years ago by 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)

comment:3 Changed 9 years ago by Ari Jolma

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

comment:4 Changed 9 years ago by 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.

comment:5 Changed 5 years ago by Jukka Rahkonen

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

comment:6 Changed 5 years ago by Ari Jolma

Resolution: worksforme
Status: newclosed

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

Note: See TracTickets for help on using tickets.