[BUG] Potential Unhandled exception in PolarisEntityResolver.java

[dbosco]

Is this a possible security vulnerability?

• This is NOT a possible security vulnerability

Describe the bug

In the following code:

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityResolver.java

Line 291 in b0eb025

PolarisEntityActiveRecord activeEntityRecord = activeRecordIt.next();

for (PolarisEntityCore resolveEntity : toResolve) {
  // get associate active record
  PolarisEntityActiveRecord activeEntityRecord = activeRecordIt.next();

  // if this entity has been dropped (null) or replaced (<> ids), then fail validation
  if (activeEntityRecord == null || activeEntityRecord.getId() != resolveEntity.getId()) {
    return false;
  }
}

If there are no elements in activeRecordIt, calling .next() will throw an exception before reaching the activeEntityRecord == null check.

Should we add a .hasNext() check before calling .next()?

If this is indeed an issue, it seems like a trivial fix, and I’d be happy to address it.

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisEntityResolver.java

Line 291 in b0eb025

PolarisEntityActiveRecord activeEntityRecord = activeRecordIt.next();

` for (PolarisEntityCore resolveEntity : toResolve) {
// get associate active record
PolarisEntityActiveRecord activeEntityRecord = activeRecordIt.next();

  // if this entity has been dropped (null) or replaced (<> ids), then fail validation
  if (activeEntityRecord == null || activeEntityRecord.getId() != resolveEntity.getId()) {
    return false;
  }
}`

If there are no elements in activeRecordIt, then the .next() will throw an exception before reaching below.
activeEntityRecord == null
Should we check for .hasNext() before calling .next().

If it is indeed an issue, it seems to be an trivial fix and I am happy to do it. Please assign it to me.

To Reproduce

No response

Actual Behavior

No response

Expected Behavior

No response

Additional context

No response

System information

No response

[eric-maynard]

I think the issue goes a little deeper than a missing hasNext. The code seems to assume that toResolve will always have the same length as activeRecordIt and that they will always be aligned (e.g. the Nth element in one corresponds to the Nth element in the other).

If this assumption is true, then we don't need the hasNext check, though we could maybe clarify this with a comment or improve readability.

If this assumption isn't true, the code needs to be fixed in more ways than with just a hasNext check.

I think the issue is this: whether or not the above assumption is true depends on the metastore implementation. Perhaps we could make lookupEntityActiveBatch return a Map, but I'm not sure whether or not we want to change the interface for this. Another approach we could take is adding a test to ensure the assumption is always true.

[dbosco]

@eric-maynard yes your correct. Assuming, the below call returns everything in the same order and length, it should be fine.
// now lookup all these entities by name Iterator<PolarisEntityActiveRecord> activeRecordIt = ms.lookupEntityActiveBatch(callCtx, entityActiveKeys).iterator();
If there is anything you want me to do, then let know. Thanks

[eric-maynard]

Personally I like the idea of making lookupEntityActiveBatch return a Map (or some other associative data structure) but I am probably more comfortable breaking APIs than some others. Without doing this, I'm not sure if there's any fix we can do right now.

[dbosco]

@eric-maynard I agree, the risk is too low. We could close this for now and revisit this at a later time.