FWD » Core Development » Base Language

#1781-15
Also, for security purposes, we should not make the resource ID algorithm a simple sequence. It should be randomized such that malicious code cannot anticipate the resource IDs that will be generated and thus cannot obtain access to a resource that they should not have.

To prevent the malicious code from guessing valid IDs they will have to be evenly distributed in a big enough range. I'm guessing that scanning the whole 64bit range (2^64 combinations) will be matter of days assuming a single validity test will take tens of CPU instructions. So if the security is real concern the IDs should be wider than 64 bits and generated by the Secure random number generator to be evenly distributed across the whole range. This leads me to Java UUID. The downside of using UUIDs of course is the higher memory and CPU impact. For example generating a random number with SecureRandom should be several tens times slower than Random.

The random 64-bit value is acceptable for now. The 4GL apps have so many other security holes that it isn't worth spending too much time on this right now.

Overall, this is very good work.

Answers

(1) "Zero" handles

As a general rule, we always implement any language feature (documented or not) that can be relied upon or otherwise seen in 4GL application behavior. In fact, I would say that much if not most of the functionality we have implemented is undocumented.

Yes, please do implement this exactly as it works in the 4GL.

(2) Resource ID deallocation

Yes, we need to solve this for sure. Constantin is working on something related right now.

Constantin: what do you think?

(3) Resource ID together with its resource?

I suspect the resource ID is logically associated with the handle, not the resource. I wonder if you could have 2 handles with different IDs referring to the same resource?

(4) The default handle string format had to be extended to cover the Long IDs. I found one test case failing (testcases/uast) due to longer ID string. Any other foreseeable issues with the new string format?

Actually, I would like you to answer this question. In particular, I would like for you to explore and document exactly where the incompatibilities exist between our approach and the 4GL. Then we can assess the impact.

Code Review 1031a

1. You will need to merge with the latest rev in bzr.

2. In handle.removeResource():

      WorkArea wa = work.get();
      Long id = wa.resourceIds.get(resource);
      wa.idHandles.remove(id);
      wa.resourceIds.remove(resource);

I think it is more appropriately coded like this:

      if (resource == null)
         return;

      WorkArea wa = work.get();
      Long id = wa.resourceIds.remove(resource);

      if (id != null)
         wa.idHandles.remove(id);

The reason for this is that we don't know that the idHandles or resourceIds maps are null-safe.

3. To answer this (from handle.resourceId():

TODO: is it enough to state that removeResource is to be called? or should we have more transparent deallocation of the entries in the resourceIds and idHandles maps?

We must have completely transparent/automatic cleanup of the related maps.

4. handle.fromResourceId() seems to be able to return null. That is used to directly return values to converted code. I don't see how that is compatible with the 4GL behavior which has no concept of an NPE. The result has to be safe and compatible. I suspect there are use cases that are missing here. The "zero handle" problem is part of this, but I worry that there are other cases too.

5. In handle.resourceId():

} while(wa.idHandles.containsKey(id)); 

should be this:

}
while (wa.idHandles.containsKey(id)); 

6. handle.fromResourceId() needs javadoc.

7. ResourceIdHelper.idToString() should return UNKNOWN_STRING instead of the literal "?".

8. ResourceIdHelper.idFromString() should respond in a 4GL compatible way when the input is out of bounds, unknown or the empty string. We can't leave this unimplemented, nor can we use Java exceptions like IllegalArgumentException since "valid" 4GL code can actually trigger these cases. What does Progress do in this case?

9. ResourceIdHelper copyright range should be 2013 only instead of a range.

10. RemoteResourceImpl, ProxyProcedureWrapper are missing header entries.

11. ServerEvent has an unnecessary indent in its history entries, please eliminate this (in your entry and the one before).

(2) Resource ID deallocation

Yes, we need to solve this for sure. Constantin is working on something related right now.

Constantin: what do you think?

The resource ID should be deallocated when the resource actually gets deleted (considering that this is the resource's ID and not the handle's - see my notes bellow).

(3) Resource ID together with its resource?

I suspect the resource ID is logically associated with the handle, not the resource. I wonder if you could have 2 handles with different IDs referring to the same resource?

Is worth mentioning here the 4GL bug (I don't think they did this on purpose) related to static QUERY resources. A static QUERY can't be deleted, but a handle referring a static QUERY can be deleted (see note 60 in #2120). This is the only case I've found in which the static resource and its associated handle don't behave the same.

In the static QUERY case, after the handle is deleted, obtaining the QUERY handle one more time will produce a different ID. I think this shows that the ID is associated with the handle. The problem is in P2J the resource and handle ID is the same thing. Unless we can prove that 2 handles with different IDs can refer the same resource, P2J can remain with the IDs linked with the resource.

(1) "Zero" handles

As a general rule, we always implement any language feature (documented or not) that can be relied upon or otherwise seen in 4GL application behavior. In fact, I would say that much if not most of the functionality we have implemented is undocumented.

Yes, please do implement this exactly as it works in the 4GL.

Ok, will do.

(3) Resource ID together with its resource?

I suspect the resource ID is logically associated with the handle, not the resource. I wonder if you could have 2 handles with different IDs referring to the same resource?

I was trying to answer the same question, looking in the Progress reference docs. No definite answer. But I would say you cannot have two handles with different IDs referring to the same resource and this is the way handle.java is coded at the moment.

(4) The default handle string format had to be extended to cover the Long IDs. I found one test case failing (testcases/uast) due to longer ID string. Any other foreseeable issues with the new string format?

Actually, I would like you to answer this question. In particular, I would like for you to explore and document exactly where the incompatibilities exist between our approach and the 4GL. Then we can assess the impact.

Will try.

Code Review 1031a

1. You will need to merge with the latest rev in bzr.

Yes, will do and will run the regression again on the merged code.

2. In handle.removeResource():

[...]

I think it is more appropriately coded like this:

[...]

The reason for this is that we don't know that the idHandles or resourceIds maps are null-safe.

Yes. I didn't realize, as you as well point out in other points below, that error handling is also subjected to the conversion and compatibility conformance. In regular Java I am used to Illegal*Exceptions sometimes even NullPointerExceptions to be part of error handling. Of course Progress conversion is another case, I will keep this in mind.

Will fix.

3. To answer this (from handle.resourceId():

TODO: is it enough to state that removeResource is to be called? or should we have more transparent deallocation of the entries in the resourceIds and idHandles maps?

We must have completely transparent/automatic cleanup of the related maps.

Will ask Constantin for a consultation on this.

4. handle.fromResourceId() seems to be able to return null. That is used to directly return values to converted code. I don't see how that is compatible with the 4GL behavior which has no concept of an NPE. The result has to be safe and compatible. I suspect there are use cases that are missing here. The "zero handle" problem is part of this, but I worry that there are other cases too.

Will fix.

5. - 11.

Will fix.

Please document the current status and upload the latest version for code review.

1. user creates a resource and saves it in a handle var
2. the handle's ID is saved in a char var
3. that var is initialized to something else
4. after a long time, the user wants to retrieve that handle via the handle function

The code would look like this:

def var h as handle.
def var ch as handle.

create sax-reader h. /* this is a resource which can be retreived only by its ID; no next/prev/first/last references are kept. */
ch = string(h).

pause 3600.

h = string(ch).
if not valid-handle(h) then message "The handle must be valid!".

Code Review 1115a

1. The explicit classes imported in handle.java should be imported with the wildcard.

2. handle.purgeStaleReferences() and the inner classes at the end of the class need javadoc (if they survive).

More broadly, I agree with Constantin's assessment. We can't rely upon weak references, the references need to be explicitly managed.

Constantin Asofiei wrote:

Are you sure that WeakReference is the way to go? What about this scenario:

1. user creates a resource and saves it in a handle var
2. the handle's ID is saved in a char var
3. that var is initialized to something else
4. after a long time, the user wants to retrieve that handle via the handle function

The code would look like this:
[...]
Idea is, there might be cases where the resource must not be implicitly deleted by the GC. I want to rely only on explicit (or implicit) resource deletion. If the a resource is long-lived, then let it live, even if no one else is referring it.

What is the boundary for the resource lifetime in this case? In other words, when should the fromString() return an invalid handle? Is it after the function exits, session ends, etc.? Is it the same for all resource types?

Are there any Progress resources subject to garbage collection? For example class instances?

Greg Shah wrote:

Code Review 1115a

1. The explicit classes imported in handle.java should be imported with the wildcard.

Fixed.

2. handle.purgeStaleReferences() and the inner classes at the end of the class need javadoc (if they survive).

More broadly, I agree with Constantin's assessment. We can't rely upon weak references, the references need to be explicitly managed.

To be resolved.

What is the boundary for the resource lifetime in this case? In other words, when should the fromString() return an invalid handle? Is it after the function exits, session ends, etc.? Is it the same for all resource types?

All resources are alive at most as long as the session is alive, and are private to that session. You don't need to worry about who deletes a resource, for this task you need to make sure that if a resource gets deleted, it is removed from the user's registry of resources.

Are there any Progress resources subject to garbage collection? For example class instances?

The class instances are not resources, thus they are not handled by handles.

What is the boundary for the resource lifetime in this case? In other words, when should the fromString() return an invalid handle? Is it after the function exits, session ends, etc.? Is it the same for all resource types?

All resources are alive at most as long as the session is alive, and are private to that session. You don't need to worry about who deletes a resource, for this task you need to make sure that if a resource gets deleted, it is removed from the user's registry of resources.

When a resource is deleted (by all means) is its instance method Deletable.delete always called? Do we care if the user doesn't explicitly delete a resource?

Are there any Progress resources subject to garbage collection? For example class instances?

The class instances are not resources, thus they are not handled by handles.

Thanks for the clarifications!

Hynek Cihlar wrote:

When a resource is deleted (by all means) is its instance method Deletable.delete always called?

• HandleChain.delete - this is the entry point for deleting almost all resources
• ExternalProgramWrapper.delete
• ProxyProcedureWrapper.delete

All these cases already have a handle.removeResource call.

Do we care if the user doesn't explicitly delete a resource?

No, if this is a non-static resource, then is user's responsibility (this is what widget pools are for, to allow mass deletion of non-static resources). For static resource cases, is P2J's responsibility (see #2196).

Constantin Asofiei wrote:

Hynek Cihlar wrote:

When a resource is deleted (by all means) is its instance method Deletable.delete always called?

Yes, that will always be called. The callpaths you need to check are:

• HandleChain.delete - this is the entry point for deleting almost all resources
• ExternalProgramWrapper.delete
• ProxyProcedureWrapper.delete

All these cases already have a handle.removeResource call.

Do we care if the user doesn't explicitly delete a resource?

No, if this is a non-static resource, then is user's responsibility (this is what widget pools are for, to allow mass deletion of non-static resources). For static resource cases, is P2J's responsibility (see #2196).

In that case the previously submitted sources (hc_upd20131031a.zip) already had all what is necessary for the resource deallocation. I will undo the weak references, regression test and submit for final review.

Hynek, the RemoteResource/RemoteResourceImpl/Agent changes are incorrect. The RemoteResource interface is used to send back to the other side the ID of a resource. When a appserver sends to the requester a handle instance, the requester receives a RemoteResource instance holding the resource ID as on the remote side; when the appserver receives from the requester a RemoteResource, it will use it to resolve the correct legacy resource. Thus, In Agent.preProcessArguments:1163, you can't use handle.resourceId, as the resource ID is already known - must be the ID specified by the RemoteResource.

The other changes look OK to me.

Constantin Asofiei wrote:

Hynek, the RemoteResource/RemoteResourceImpl/Agent changes are incorrect. The RemoteResource interface is used to send back to the other side the ID of a resource. When a appserver sends to the requester a handle instance, the requester receives a RemoteResource instance holding the resource ID as on the remote side; when the appserver receives from the requester a RemoteResource, it will use it to resolve the correct legacy resource. Thus, In Agent.preProcessArguments:1163, you can't use handle.resourceId, as the resource ID is already known - must be the ID specified by the RemoteResource.

The other changes look OK to me.

Thank you Constantin for the catch. This is a piece of code related to a version with resource ids being saved directly in the resources, I forgot to remove it.

I've run regression test and will post the fixed changes when the test finishes.

Hynek Cihlar wrote:

... I am attaching the latest changes with the fixed RemoteResource/RemoteResourceImpl/Agent, please review.

The update is OK, as you've backed out the Agent.java, ProxyProcedureWrapper.java, RemoteResourceImpl.java and RemoteResource.java changes.

The code (hc_upd20131118a.zip) passed regression tests (the run 20131116) and was committed as revision 10416.

Some socket-related static proxy methods got bad after the 1118a.zip update, as some signatures are out-of-sync. I'm working on fixing them.

Greg: after I've added logging to the static proxy registration, I found these too:

[12/18/2013 11:49:54 EET] (RemoteObject:SEVERE) Unable find matching static method public abstract boolean com.goldencode.p2j.util.RemoteErrorData.isError() in backing class com.goldencode.p2j.util.ErrorManager.
[12/18/2013 11:49:54 EET] (RemoteObject:SEVERE) Unable find matching static method public abstract void com.goldencode.p2j.util.RemoteErrorData.setDontClean(boolean) in backing class com.goldencode.p2j.util.ErrorManager.
[12/18/2013 11:49:54 EET] (RemoteObject:SEVERE) Unable find matching static method public abstract boolean com.goldencode.p2j.admin.AdminExports.changeUserGroups(java.lang.String,java.lang.String[],java.lang.String[]) in backing class com.goldencode.p2j.admin.AdminServerImpl.

I should have caught this earlier. Why are negative resource ids ignored in ResourceIdHelper.idFromString?

      // we only consider positive integers valid resource ids
      int signum = num.signum();
      if (signum <= 0)
      {
         return ZERO;
      }

Code Review 1218a

The changes all look good.

The LT will need merging with the one just checked in by VIG.

In regard to the errors reported in note 27, go ahead and fix those issues and include the change in your next update (it can be for another task if 1218a has already passed testing).

Constantin Asofiei wrote:

Fixed signatures of static proxy methods. Added logging to RemoteObject.registerStaticNetworkServer, to report any unresolved proxied methods (the problems at note 27 are still reported).
LE: fixed a socket problem too.

Hynek: about the negative resource IDs, please let me know your opinion on the following:

• I think ResourceIdHelper.getNext should never return negative values
• with this in mind, ResourceIdHelper.idFromString and ResourceIdHelper.idToString should throw an exception if a negative ID is encountered. This looks OK, because if we change getNext to return always positive values, then these methods should never received a negative ID (as the only way to compute a resource ID is via getNext).

The idea behind the ids was to utilize the whole 64 bits in the Long. And because Progress always formats the ids as positive numbers so does the ResourceIdHelper.idToString. The cycle Long Id -> String Id -> Long Id works as long as you use the methods from ResourceIdHelper to format the id to a string and to parse it back to a long.

Example (using byte instead of long to preserve space):

byte newId = ResourceIdHelper.getNext()
// newId == -1
String formattedNewId = ResourceIdHelper.idToString(newId)
// formattedNewId == "255" 
byte parsedNewId = ResourceIdHelper.idFromString(formattedNewId)
// parsedNewId == -1

This all was done to use a primitive type to represent the id (to make it less memory intensive and preserve some GC cycles) while utilizing the whole bit-range.

The biggest disadvantage of this scheme is the added complexity and a potential room for errors.

Constantin, your proposed solution will work ok, we just lose half of the id set.

Constantin Asofiei wrote:

I should have caught this earlier. Why are negative resource ids ignored in ResourceIdHelper.idFromString?

The reason for this is that Progress returns "zero" handle when negative number is passed to the HANDLE function. But I agree the implementation is confusing. This should probably be implemented in the handle class, there it would be more obvious.

Code Review 1219b

I am OK with the changes. If Hynek is OK with them, then go ahead with testing.

Constantin, I would remove the doc references about treating the long as unsigned number, now we use it as plain signed long, together with removing the ResourceIdHelper.TWO_64 since it is useless now.

Also when you call handle.fromString() with a negative number, an exception is thrown instead of "zero" handle being returned.

Otherwise it looks ok.

Hynek Cihlar wrote:

Constantin, I would remove the doc references about treating the long as unsigned number, now we use it as plain signed long, together with removing the ResourceIdHelper.TWO_64 since it is useless now.

OK, will do.

Also when you call handle.fromString() with a negative number, an exception is thrown instead of "zero" handle being returned.

I'm not sure throwing an exception in this case is correct. As you noted, when a negative value is passed to a HANDLE function call, 4GL will generate the ZERO handle.

Constantin Asofiei wrote:

Also when you call handle.fromString() with a negative number, an exception is thrown instead of "zero" handle being returned.

I'm not sure throwing an exception in this case is correct. As you noted, when a negative value is passed to a HANDLE function call, 4GL will generate the ZERO handle.

Sorry, I didn't express myself clearly. With your change, when "-1" is passed to handle.fromString(), IllegalArgumentException is thrown. The expected behavior is however that a "zero" handle instance is returned.

Hynek Cihlar wrote:

Sorry, I didn't express myself clearly. With your change, when "-1" is passed to handle.fromString(), IllegalArgumentException is thrown. The expected behavior is however that a "zero" handle instance is returned.

Hmm... handle.fromString calls ResourceIdHelper.idFromString, and the version from 1219b.zip doesn't throw an exception. Only ResourceIdHelper.idToString throws one, which is called from handle.toString() (the equivalent of the STRING function).

True, I was looking at the other method. Please ignore the comment about throwing an exception when calling handle.fromString("-1").

OK, one more question about the TWO_64 usage. In ResourceIdHelper.idFromString, you have this code (btw, shouldn't have been modulo(TWO_64) instead of substract?):

      // out of range of positive long?
      if (num.compareTo(MAX_LONG) > 0)
      {
         // make the value fit into the 2s complement long
         num.subtract(TWO_64);
      }

AFAIK, the subtract is correct for the 2s complement, but the code is missing check for value greater than max unsigned long, i.e. the range check you proposed.

• Progress reports the error "** Value too large for integer. (78)" when the string value doesn't fit into the range of an unsigned 64bit number.
• And it reports an additional error ("** Decimal number is too large. (536)\n** Value too large for integer. (78)") when the string exceeds 50 significant digits (i.e. not including numsign, white spaces, leading zeros, etc.).
• Plus when the length of the input string exceeds 255 characters, "ZERO" handle is returned.

I think, the proper errors should be returned instead of throwing exception.

Code Review 1220a

I am fine with the changes. I think that these errors are really things that are generically raised in the decimal/integer code in the 4GL. I am actually hitting some of the same error requirements when processing the output/return data from the native API calls. We should probably find the proper common location for the error processing.

But for this task, go with your current implementation.

1220a.zip passed testing, was committed to bzr rev 10424.