FWD » Core Development » Base Language

Proper processing of Client Platform Dependencies requires these features to be implemented in a separate client JVM process.

However, there are definitely cases in which these same features could be implemented in a purely server-side manner and converted code could uses them within the FWD JVM without a problem. For this purpose, we will enable usage of some of the client platform features from within the server process. Where this can be safely done, it may represent a major improvement in performance.

One can easily consider these cases (maybe others too):

• file system access and streams
• XML to/from file
• web services and sockets usage

We would implement 4GL syntax support for forcing the processing to be server side. Some minor conversion and runtime changes are needed, but most of the support would easily move to the server side without much effort.

The only other issue here is that we need to implement a security manager plugin to secure access to the server side resources. At least the filesystem resources need to be protected this way. Otherwise there might be major unintended access for users to data that should be secured on a server but now might be exposed.

See also some discussion in #3254-4.

A key part of the solution will be to implement a server-side object instead of the normal client-side remote proxy that is normally used. This will allow transparent usage of the server side without any other changes.

That server-side object would need to have the security manager checks built in.

For those client dependencies which cannot and should not be implemented, we can implement a kind of mock implementation that does nothing. This would "null out" any usage safely.

I'd like to get an estimate of the performance improvement this task can offer for non-interactive use cases. Perhaps the JMX instrumentation of the DAP can be used to make an estimate?

On the other hand, the effort to do a quick implementation (without a security manager plugin) is probably not too bad. Doing a first pass would be even more effective to see the results.

The implementation of #4065-5 (without security) seems pretty straightforward. The less obvious part will be eliminating any ThinClient or other client-specific code dependencies from the backing workers so that they can run on either the client or server.

• do we want to have a hybrid model where some parts exist on the client? If yes, then the client JVM process is still needed. The only useful case I see for a hybrid model is to have interactive clients which allocate the memptrs, open client sockets, call web services, on the server-side, and not on the client-side. File access is more tricky to move to server-side, as we need the security plugins to 'sandbox' them. And something I haven't considered until now - server sockets can't be opened by multiple clients if they ran on the same machine. This may be considered a limitation, which can be partially solved with remote brokers to launch the client on a different machine.
• if we don't need a hybrid approach, my first thought is to have a mode similar to the process:arch:single=true, where the client is ran as a thread in the server's JVM. But currently this is broken, as SessionManager.get().isLeaf() will return false, becauses the SessionManager.instance is a JVM-wide singleton, set to a RouterSessionManager instance. Regardless if the client APIs are executed in a different thread or in the same Conversation thread on the server-side, we will need some changes in client-side classes which rely on isLeaf() to determine if is the client or server using it.
• the hybrid approach also has some other complexities, where we need to identify client-side resources which now are implemented on the server-side, and what are other client-side dependencies which still need access to those resources (which now exist on server-side). Think memptr, are there cases where some client API has a memptr parameter? If so, these APIs will need to have a 'reverse-proxy' back to the server, to access it, or otherwise refactor them to execute on server-side.
• you mention a 'server-side object'. We already have RemoteObject.obtainLocalInstance to obtain a 'local proxy'. What would be different for this 'server-side object'?
• batch/appserver clients started via the FWD server (and not manually or via a remote broker client) can be marked to use this 'server-side mode' always.

do we want to have a hybrid model where some parts exist on the client? If yes, then the client JVM process is still needed. The only useful case I see for a hybrid model is to have interactive clients which allocate the memptrs, open client sockets, call web services, on the server-side, and not on the client-side.

We may need a hybrid model for future requirements. I don't expect the REST cases to require it, so it may be best to do this in 2 phases with the hybrid model left for a 2nd phase.

I do see the hybrid model as being needed for more than just interactive cases. All it takes is one shared library that is not thread-safe, or a case where a server socket must listen on the same port for more than one session... this can be as likely for non-interactive code as for interactive.

if we don't need a hybrid approach, my first thought is to have a mode similar to the process:arch:single=true, where the client is ran as a thread in the server's JVM.

I don't see this as a good solution. This must coexist in a system where real client JVMs still operate. In other words, this should be a session level configuration.

BTW, I think we would be better off removing the process:arch:single=true mode. We never use it and it just complicates the code. We don't have to do it here, but then again we'll be editing the same locations anyway.

you mention a 'server-side object'. We already have RemoteObject.obtainLocalInstance to obtain a 'local proxy'. What would be different for this 'server-side object'?

Yes, this is the idea. The point is that we must instantiate the equivalent of MemoryDaemon, StreamDaemon, FileSystemDaemon... on the server side and use those instead of the remote version in those sessions which are configured.

batch/appserver clients started via the FWD server (and not manually or via a remote broker client) can be marked to use this 'server-side mode' always.

It should probably be optional so that we have a "sure to be compatible" option. Perhaps we make this the default but allow the session to be configured to use the spawner and create a client JVM if so configured.

I have some changes which brings memptr fully on server-side (very pin-pointed changes, no major refactoring).

• with memptr fully on server-side, a copy-lob or any other statement which uses a memptr and another client-side resource, now will have to call-back on the server-side to access the memptr.
• server-side will need to initialize network proxies for MemoryDaemon and LibraryDaemon (here what I really need is the libp2j.so loading from LibraryDaemon, I think I'll move that in a separate method to be able to load it explicitly).
• LeafSessionManager.getSession() needs to return non-null, so activeSession is set in connectDirect. This required some small patches in some classes which created a network object only if session != null, instead of relying on SessionManager.isLeaf(). Also, this required RemoteObject.obtainInstance to first look on a local instance and fallback to network server only if there is no local instance.
• how should we enable the server-side memptr? Via a directory configuration? A bootstrap configuration? Should we allow 'pick and choose' which resources are on server-side and which are on client-side? This affects how the client-side can access this resource, if is moved on server.

I assume (for now) that you are not removing the ability to use memptr on the client. Perhaps we will find that long term it can be server only. To do so would require changes to ensure that client side usage of copy-lob and library calls have their memptr requirements met dynamically.

• with memptr fully on server-side, a copy-lob or any other statement which uses a memptr and another client-side resource, now will have to call-back on the server-side to access the memptr.

Please mark those APIs (with TODOs) which need re-optimization. Where possible, we will want to rework the downcalls in these cases to pass the data needed.

• server-side will need to initialize network proxies for MemoryDaemon and LibraryDaemon (here what I really need is the libp2j.so loading from LibraryDaemon, I think I'll move that in a separate method to be able to load it explicitly).

Yes, that makes sense.

I think we have to add some memptr code for this server-side case to limit access to pointers which were allocated for a different session. In other words, it is a security breach if some code tries to access arbitrary memory locations. This is possible using SET-POINTER-VALUE() which is very dangerous on the server side. We need some kind of per-session registry of valid memory regions (base address, size). This would hold regions directly allocated by the session.

I think we have a problem with memory allocated by a library call. This can happen with output parameters, for which we can dynamically setup a region. The problem is for library calls that return an address (an integer or int64) in a structure where that address represents a pointer to memory that has been allocated. In such cases the memory region is available in process, but we don't know about it. SET-POINTER-VALUE() must be to be disallowed by default on the server unless it is using an address in our list of known valid regions.

If customers have code that breaks from this restriction, we can provide a mechanism to configure a bypass for specific code. I would wait to implement that additional feature until we find a customer that needs it.

Eugenie: Please do a code review of our current libp2j implementation to try to find any issues related to thread safety. I'm concerned that we have not written the code there to gracefully/correctly be executed on the server in multuple threads. The signal processing and pseudo-terminal processing comes to mind for me, as needing some update.

• LeafSessionManager.getSession() needs to return non-null, so activeSession is set in connectDirect. This required some small patches in some classes which created a network object only if session != null, instead of relying on SessionManager.isLeaf(). Also, this required RemoteObject.obtainInstance to first look on a local instance and fallback to network server only if there is no local instance.

OK. I'm also OK if we want to eliminate the "single session" mode. We never use it and it is a complication that should probably be removed.

• how should we enable the server-side memptr? Via a directory configuration? A bootstrap configuration? Should we allow 'pick and choose' which resources are on server-side and which are on client-side? This affects how the client-side can access this resource, if is moved on server.

This cannot be a bootstrap configuration thing, as that would open a security hole (any client could set this and open up access on the server side). This must be configured in the directory. I think it is enough for now that the usage is activated on a user/group/server/default basis. Later, we may need some granularity by resource to have some things on the client and others on the server, but for now let's assume it is all one way or the other.

Greg Shah wrote:

I assume (for now) that you are not removing the ability to use memptr on the client. Perhaps we will find that long term it can be server only. To do so would require changes to ensure that client side usage of copy-lob and library calls have their memptr requirements met dynamically.

Exactly, when memptr is on server-side, client-side will need to call the server for any memptr usage.

• with memptr fully on server-side, a copy-lob or any other statement which uses a memptr and another client-side resource, now will have to call-back on the server-side to access the memptr.

Please mark those APIs (with TODOs) which need re-optimization. Where possible, we will want to rework the downcalls in these cases to pass the data needed.

OK

I think we have to add some memptr code for this server-side case to limit access to pointers which were allocated for a different session. In other words, it is a security breach if some code tries to access arbitrary memory locations. This is possible using SET-POINTER-VALUE() which is very dangerous on the server side. We need some kind of per-session registry of valid memory regions (base address, size). This would hold regions directly allocated by the session.

I think we have a problem with memory allocated by a library call. This can happen with output parameters, for which we can dynamically setup a region. The problem is for library calls that return an address (an integer or int64) in a structure where that address represents a pointer to memory that has been allocated. In such cases the memory region is available in process, but we don't know about it. SET-POINTER-VALUE() must be to be disallowed by default on the server unless it is using an address in our list of known valid regions.

Good points, I'll add them.

This cannot be a bootstrap configuration thing, as that would open a security hole (any client could set this and open up access on the server side). This must be configured in the directory. I think it is enough for now that the usage is activated on a user/group/server/default basis. Later, we may need some granularity by resource to have some things on the client and others on the server, but for now let's assume it is all one way or the other.

Understood, thank you.

• should I add support for server-side LibraryDaemon together with memptr? They are tightly coupled in the p2j.library package.
• I'm using RoaringBitmap to keep the 'in use' address space for each client context, on server-side.
• the directory configuration can be named server-side-resources and keep a list of FWD qualified class names, like com.goldencode.p2j.util.memptr - or do you want something more user-friendly?

Also, is it enough to raise an ERROR condition if the application code tries a set-pointer-value() to something outside the context's known address space?

should I add support for server-side LibraryDaemon together with memptr? They are tightly coupled in the p2j.library package.

Yes

do you want something more user-friendly?

A single string with comma separated values will keep it simple.

                <node class="string" name="server-side-resources">
                  <node-attribute name="value" value="memptr,library,streams"/>
                </node>

The names don't have to match classes, they should just make sense for the feature.

is it enough to raise an ERROR condition if the application code tries a set-pointer-value() to something outside the context's known address space?

Yes, I think that is OK for now. Make sure that we log it too, since these are places that need some attention.

The code is complete for memptr and library, I need to finish a testing of #6075 and some other scenarios.

I've added a OSResourceManager class with two sub-classes, ClientSideResourceManager and ServerSideResourceManager. This contains APIs to be used on client, server or common client-server code, to get the proper network or local instance, or to initialize the resource properly (either on client or server-side, by creating the network or local proxy).

Code Review Task branch 3821c Revision 13885

Overall, I very much like the changes.

1. I don't fully understand the use of statics in OSResourceManager (and subclasses). Its data seems like it will always contain global state across the server or client. In other words, on the server, there is no per-session capability. In addition, it is not thread-safe when used in this way. Am I mis-understanding this?

2. In regard to the thread-safety of OSResourceManager (and subclasses), even if we make this per-context we should eventually implement this using synchronization. It is likely that it will be called in a multi-threaded environment and I would prefer if it will be safe. If we don't implement this now, then we need to remember it is technical debt. Perhaps we need some TODO comments to this effect.

3. In the static initializer for OSResourceManager, the resource names should be processed case-insensitively.

4. Any usage of streams/sockets which remains client-side and needs memptr access will be broken now, right? I don't see any rework to up-call or to pass the data down more efficiently.

Roger: As part of rev 13885, the RoaringBitmap library version has changed from 0.9.0 to 0.9.27. Would you please handle the version number changes in our documentation/license project?

Greg Shah wrote:

1. I don't fully understand the use of statics in OSResourceManager (and subclasses). Its data seems like it will always contain global state across the server or client. In other words, on the server, there is no per-session capability. In addition, it is not thread-safe when used in this way. Am I mis-understanding this?

There is no context-local state to be kept. These maps are used only when initializing the ClientSideResourceManager or ServerSideResourceManager, which is done only once per JVM (at server or client startup). Every other access is read-only.

But I see what you mean now, the code is not working properly on a 'per-user' basis... I need to fix this.

3. In the static initializer for OSResourceManager, the resource names should be processed case-insensitively.

The server-side-resources string is lowercased before splitting it into tokens.

4. Any usage of streams/sockets which remains client-side and needs memptr access will be broken now, right? I don't see any rework to up-call or to pass the data down more efficiently.

No, is not broken, client-side will call into server-side, there are TODOs in place where optimizations may be needed.

I've fixed the per-context support of server-side-resources in 3821c/13886.

Code Review Task Branch 3821c Revision 13886

It looks good.

Greg Shah wrote:

Roger: As part of rev 13885, the RoaringBitmap library version has changed from 0.9.0 to 0.9.27. Would you please handle the version number changes in our documentation/license project?

Sure thing.

Roger Borrello wrote:

Greg Shah wrote:

Roger: As part of rev 13885, the RoaringBitmap library version has changed from 0.9.0 to 0.9.27. Would you please handle the version number changes in our documentation/license project?

Sure thing.

Done. Please rename RoaringBitmap-0.9.0.jar_LICENSE.txt to RoaringBitmap-0.9.27.jar_LICENSE.txt on the web server: https://proj.goldencode.com/artifacts/licenses/

Please rename RoaringBitmap-0.9.0.jar_LICENSE.txt to RoaringBitmap-0.9.27.jar_LICENSE.txt on the web server: https://proj.goldencode.com/artifacts/licenses/

Done.

There is an issue with server-side memptr which affects #6277. The problem is related to memptr used at define return parameter, define output parameter at the native call.

For DEFINE RETURN, this fails at LibraryManager.invoke:331:

      // copy back return value
      if (retval != null)
      {
         retval.restore(retaddr);
      }

with Memory violation: trying to SET-POINTER-VALUE to an address unallocated in this context.:

memptr.checkAddressSpace(long) line: 1497    
memptr.setPointerValue(long) line: 506    
NativeBuffer.restore(long) line: 257    
LibraryManager.invoke(String, String, int, boolean, CallingConvention, Signature) line: 331    

I see memptr.setPointerValue is used at:

NativeBuffer.render(long)
NativeBuffer.restore(long)
NativePrimitive.render(long)
NativePrimitive.restore(long)
NativeString.render(long)
NativeTypeArray.render(long)
NativeTypeArray.restore(long)

An approach would be to force registration of this pointer in the FWD context's address space (as it was returned by an API call), in the cases above. But the deregistration of this pointer from the FWD context's address space may not happen, if the memptr is not deallocated in the Java code, but in another native library call...

Another approach would be to allow memptr to be assigned addresses returned by the native API calls, and not register them in the FWD context's address space, when server-side memptr is used.

But the deregistration of this pointer from the FWD context's address space may not happen, if the memptr is not deallocated in the Java code, but in another native library call...

This is a common case. Native libraries often allocate memory and return a pointer. This allocation could be done via CRT functions, OS functions or other means. The caller must not attempt to deallocate such pointers.

An approach would be to force registration of this pointer in the FWD context's address space (as it was returned by an API call), in the cases above.

This may make sense. The biggest issue is that the size of the memory area is unknown to us.

Another approach would be to allow memptr to be assigned addresses returned by the native API calls, and not register them in the FWD context's address space, when server-side memptr is used.

I think we must make an effort to avoid protection faults since such a problem will crash the server JVM. If we have no registration or protection, how do we avoid such crashes?

Perhaps we should require that the 4GL code is modified to tell us the size of the returned pointer when SET-POINTER-VALUE is called. This is something that would be case by case (it depends on the native library call itself and sometimes may be based on data structure size and other times on some size value that is passed in). Either way, the caller will know this information and we cannot deduce it ourselves. With that information, I think we can protect the server.

Greg Shah wrote:

Perhaps we should require that the 4GL code is modified to tell us the size of the returned pointer when SET-POINTER-VALUE is called. This is something that would be case by case (it depends on the native library call itself and sometimes may be based on data structure size and other times on some size value that is passed in). Either way, the caller will know this information and we cannot deduce it ourselves. With that information, I think we can protect the server.

For this specific case, is not about 4GL SET-POINTER-VALUE - is about internal FWD assignment of the memptr pointer, as returned via a RETURN or OUTPUT parameter.

Currently, when SET-POINTER-VALUE is called, any address in the context's allocated address space is allowed. We don't reserve address space on SET-POINTER-VALUE.

And you raise a good point - there is no knowledge about the size of the address from RETURN or OUTPUT params at the native call, so I can't allocate this in the FWD context's address space.

The only solution I see is to disable the address space check for RETURN and OUTPUT parameters, in the FWD runtime.

The only solution I see is to disable the address space check for RETURN and OUTPUT parameters, in the FWD runtime.

I don't think we can do this. It is important that each application has been carefully vetted to ensure it is safe for server-side resource usage. Dangerous operations should be reviewed to confirm. If that requires 4GL changes to inform FWD of how to make this safe, it is OK with me.

In this case, we can augment the RETURN or OUTPUT parameter with additional size information. Actually it is probably better to add a call to register the externally allocated pointer (and size of the memory allocation) in FWD. By making this explicit, we tell FWD that it is OK to allow access to that range.

Greg Shah wrote:

In this case, we can augment the RETURN or OUTPUT parameter with additional size information. Actually it is probably better to add a call to register the externally allocated pointer (and size of the memory allocation) in FWD. By making this explicit, we tell FWD that it is OK to allow access to that range.

I understand, but I have at least a case in #6277 where the RETURN parameter is not being deallocated on the FWD server side, I assume is being deallocated by the native library call (in this case, the library call is https://www.openssl.org/docs/man1.1.1/man3/EVP_sha1.html). My point is if the memory is not deallocated on FWD server, this will lead to a memory leak in the FWD's allocated address space. So this needs to be solved, too, somehow - maybe directly in the 4GL code?

Constantin Asofiei wrote:

... in this case, the library call is https://www.openssl.org/docs/man1.1.1/man3/EVP_sha1.html

I mean this is the library call which returns the pointer to FWD, and after that the pointer is set as first argument to https://www.openssl.org/docs/man1.1.1/man3/HMAC.html

I understand, but I have at least a case in #6277 where the RETURN parameter is not being deallocated on the FWD server side, I assume is being deallocated by the native library call (in this case

This is the common case for return and output pointers.

My point is if the memory is not deallocated on FWD server, this will lead to a memory leak in the FWD's allocated address space. So this needs to be solved, too, somehow - maybe directly in the 4GL code?

This is not something we can handle in FWD. If there is a memory leak by using a native library, then that leak will exist in both FWD and in OpenEdge. Although we would like there to be no leak, it is not required that we resolve this. I'm more worried about avoiding protection violations.

Greg Shah wrote:

My point is if the memory is not deallocated on FWD server, this will lead to a memory leak in the FWD's allocated address space. So this needs to be solved, too, somehow - maybe directly in the 4GL code?

This is not something we can handle in FWD. If there is a memory leak by using a native library, then that leak will exist in both FWD and in OpenEdge. Although we would like there to be no leak, it is not required that we resolve this.

Here I mean if the memory is allocated and deallocated by a native library call, FWD will keep it allocated in the context's address space, as it 'allocated' this address space when the RETURN or OUTPUT parameter was used - so this is a FWD leak, not an application leak.

I'm more worried about avoiding protection violations.

Understood.

FWD will keep it allocated in the context's address space, as it 'allocated' this address space when the RETURN or OUTPUT parameter was used - so this is a FWD leak, not an application leak.

We don't actually allocate any additional memory, but there will be an extra entry in our registry. Is that what you mean?

Greg Shah wrote:

We don't actually allocate any additional memory, but there will be an extra entry in our registry. Is that what you mean?

Exactly. FWD's allocated address space registry will end up have unallocated memory marked as allocated.

Yes, this is a flaw but it is better than the alternative.

Can we detect when the same lvalue is re-assigned a new pointer value (on return or output) and automatically deregister at that time? That may lessen the chances of a significant leak.

I'm currently working on the file-system server-side processing. Here are a few notables:

• Current FWD security model doesn't have primitives for file-system resources, this will have to be extended.
• The file system security plugin (FileSystemResource) will be based on BitFlagsResource.
• The actual file-system security model will be based on the Linux file-system security model.
• File system resources names will be defined as absolute file names. With the option to specify a first-level subdir (*) or any-level subdirs (**).
• The security plugin will be in effect only on server-side, not on the clients.

Are the above reasonable?

I created task branch 4065a based on trunk revision 14477.

Current FWD security model doesn't have primitives for file-system resources, this will have to be extended.
The file system security plugin (FileSystemResource) will be based on BitFlagsResource.

Yes on both.

The actual file-system security model will be based on the Linux file-system security model.

Can you clarify what you mean here? The BitFlagsResource has flags CDRWN (create, delete, read, write, denied) and these are the correct permissions that we should support. The Linux file system permissions are too weak in comparison. Likewise, we don't need to implement the concept of owner, group, others because our security model defines a much richer set of possible cases. Any number of accounts including processes/servers, groups and others can all be defined and working together. I don't want to limit us to a Linux approach. That is not a good model.

I'm not a fan of the Windows drive letter either, but I think we cannot avoid dealing with it. I hope we can hide some of that system-specific nonsense behind some core J2SE classes like File.

File system resources names will be defined as absolute file names.

I don't want to limit us to absolute filenames. In fact, I think it would be a bad practice to encode these because moving the server location (current directory) would then require potentially major changes to the directory. When one considers that a single converted application will be installed in potentially thousands of different locations, having to manage absolute filenames will be painful.

If a filename starts with / or a drive letter + colon, then it is absolute. Otherwise it is considered relative to the current directory. This will allow us to support fully relative projects, where some portion of the server file system is opened up for access but the rest is inaccessible.

With the option to specify a first-level subdir () or any-level subdirs (*).

Good. We probably should support the other normal "globbing" character ?.

The security plugin will be in effect only on server-side, not on the clients.

Yes

Greg Shah wrote:

The actual file-system security model will be based on the Linux file-system security model.

Can you clarify what you mean here?

I was referring to the simplicity of bitset values set on the file system objects. The actual set of permissions is as you say. Except I think we should implicitly deny access when no permission rule is matched. With the above I'm not sure what should be the semantic of the DENIED permission. Also we should introduce the EXECUTE permission.

File system resources names will be defined as absolute file names.

I don't want to limit us to absolute filenames. In fact, I think it would be a bad practice to encode these because moving the server location (current directory) would then require potentially major changes to the directory. When one considers that a single converted application will be installed in potentially thousands of different locations, having to manage absolute filenames will be painful.

If a filename starts with / or a drive letter + colon, then it is absolute. Otherwise it is considered relative to the current directory. This will allow us to support fully relative projects, where some portion of the server file system is opened up for access but the rest is inaccessible.

OK.

With the option to specify a first-level subdir () or any-level subdirs (*).

Good. We probably should support the other normal "globbing" character ?.

OK.

Hynek Cihlar wrote:

Also we should introduce the EXECUTE permission.

I will rephrase this to a question. Should we pursue some kind of EXECUTE permissions on the OS-COMMAND statement?

Except I think we should implicitly deny access when no permission rule is matched.

Agreed. We always should assume no access unless explicitly granted.

With the above I'm not sure what should be the semantic of the DENIED permission.

It is still needed in the case where existing rules would otherwise grant access. In this way you can exclude items from a larger set of included items.

Also we should introduce the EXECUTE permission.

I like this, yes.

Should we pursue some kind of EXECUTE permissions on the OS-COMMAND statement?

It is a really good idea.

I think you can add this EXECUTE extra bit into the BitFlagsResource directory.

Greg Shah wrote:

Should we pursue some kind of EXECUTE permissions on the OS-COMMAND statement?

It is a really good idea.

The question is how to enforce the permission. The command may be any expression interpreted by the system. We could just do a regexp match on the value.

Should we pursue some kind of EXECUTE permissions on the OS-COMMAND statement?

It is a really good idea.

The question is how to enforce the permission. The command may be any expression interpreted by the system. We could just do a regexp match on the value.

I think we focus on the command itself and ignore any parameters. The code in ProcessOps will have the evaluated expression that includes the command itself as the leftmost text. At that time we can lookup the command to ensure that we have EXECUTE access before allowing the OS-COMMAND to continue. Treat it like any other relative/absolute filename. If it matches with a rule that includes EXECUTE, then it can run. Otherwise we raise an ERROR condition.

Hynek Cihlar wrote:

I created task branch 4065a based on trunk revision 14477.

FYI, trunk is on rev 14483.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

I created task branch 4065a based on trunk revision 14477.

FYI, trunk is on rev 14483.

Thanks for the correction. My local branch is older, 4065a is indeed branched off 14483.

Greg, do we want to interpolate the file paths with a set of known variables? Like logged in user, application name, working dir, etc? This would make the system easier to manage.

Hynek Cihlar wrote:

Greg, do we want to interpolate the file paths with a set of known variables? Like logged in user, application name, working dir, etc? This would make the system easier to manage.

Yes, that makes sense. This is pretty easy to do in the SecurityManager plugin.

Greg, I found several places in the client UI where file IO is performed directly. An example is a button widget loading an image.

With respect to the anticipated use cases does it make sense to replace these with the managed file system resource calls?

With respect to the anticipated use cases does it make sense to replace these with the managed file system resource calls?

Yes.

Doing it that way would also allow us to intercept and redirect resources from the jar in a cleaner way. Today we've inserted some "one off" ways to load resources from the jar but really it would be better as an architected and standard feature.

I rebased 4065a against trunk revision 14520.

From Hynek via email:

Changing the direct IO in the client process to go through a remoted object on the server will make the performance worse. Or do I miss something?

Please describe what this refers to. You mean resources which are required by the client-side (like images for GUI and stuff like that)?

Constantin Asofiei wrote:

From Hynek via email:

Changing the direct IO in the client process to go through a remoted object on the server will make the performance worse. Or do I miss something?

Please describe what this refers to. You mean resources which are required by the client-side (like images for GUI and stuff like that)?

Yes, I meant resources needed by the client side.

• there is EDITOR:READ-FILE, which can't be on the server-side, as the FWD client will need to go over the network to read this.
• other usage like image loading, etc, which are on the client-side only.

Constantin Asofiei wrote:

Greg, correct me if I'm wrong, but server-side processing is required only by legacy streams (used by INPUT/OUTPUT statements) and other cases where the stream doesn't interact with the FWD Client; for example:

• there is EDITOR:READ-FILE, which can't be on the server-side, as the FWD client will need to go over the network to read this.
• other usage like image loading, etc, which are on the client-side only.

Constantin, see #4065-59 and the related where we discussed this.

Hynek Cihlar wrote:

Constantin, see #4065-59 and the related where we discussed this.

OK, but when server-side streams are active, the client should not call the server-side and read the file, over the socket, if this is a 'client-only' file. It will add unnecessary overhead IMO.

Constantin Asofiei wrote:

Hynek Cihlar wrote:

Constantin, see #4065-59 and the related where we discussed this.

OK, but when server-side streams are active, the client should not call the server-side and read the file, over the socket, if this is a 'client-only' file. It will add unnecessary overhead IMO.

This was my concern, too and this why I wanted to descope it.

I have a question about this task... In #4938-52 I need to configure the web client such that we have this in the directory:

    <node class="container" name="server">
      <node class="container" name="default">
        <node class="container" name="webClient">
          <node class="string" name="configFile">
            <node-attribute name="value" value="user_client.xml"/>
          </node>
          <node class="string" name="workingDir">
            <node-attribute name="value" value="~/.app-cfg"/>
          </node>

We'd need the ~ to be resolved to the client's homespace (not the server's) so that we can load cmd-line-option ininame= from a /home/<user>/.app-cfg/user_client.xml, which the application can modify based upon user preference. Using the tilde allows the same directory to work for any user accessing from a web browser and signing into the OS. Perhaps $HOME could be used, but the point is, this needs to come from the client's filesystem.

Should this be working already, without changes going into this task? Because I see the value being read at server startup (I temporarily modified the code to handle tilde) so when the web-client starts, it is already using a value determined on the server's filesystem.

Roger Borrello wrote:

I have a question about this task... In #4938-52 I need to configure the web client such that we have this in the directory:
[...]

We'd need the ~ to be resolved to the client's homespace (not the server's) so that we can load cmd-line-option ininame= from a /home/<user>/.app-cfg/user_client.xml, which the application can modify based upon user preference. Using the tilde allows the same directory to work for any user accessing from a web browser and signing into the OS. Perhaps $HOME could be used, but the point is, this needs to come from the client's filesystem.

Should this be working already, without changes going into this task? Because I see the value being read at server startup (I temporarily modified the code to handle tilde) so when the web-client starts, it is already using a value determined on the server's filesystem.

Currently we don't interpolate the workingDir directory value, also I don't think you want that. To me it looks the resolution of the ini file itself should be enhanced regardless of workingDir.

If we look at how the options in the ini file (and the registry) are resolved, we should probably also consider to allow the ini values resolution to check a user specific location. OE checks the current user registry hive (HKCU) and then local machine (HCLM) Windows registry hive when resolving a specific variable. If we follow this logic, then we should consider a user specific location of the client process system account (home?) before resolving the ini file against the working dir. Also the options from the user-specific dir and working dir should be merged with the user-specific taking precedence. This is because OE does this resolution per value.

Greg, what do you think?

but server-side processing is required only by legacy streams (used by INPUT/OUTPUT statements) and other cases where the stream doesn't interact with the FWD Client

Correct

but when server-side streams are active, the client should not call the server-side and read the file, over the socket, if this is a 'client-only' file. It will add unnecessary overhead IMO.

This was my concern, too and this why I wanted to descope it.

This was never in scope, so it doesn't need to be a separate task. We aren't doing that.

what do you think?

I think this is a discussion that is unrelated to this task. Yes, we probably have a bug in how we resolve the web client working dir. That has nothing to do with this task.

As far as the INI/registry support, we can discuss that elsewhere. We'll have to write some code to check our compatibility and then fix any issues. Roger's problem doesn't relate to the INI/registry support. It is really something to discuss and fix in #4938. In that task we have to load a per-user bootstrap config file located in the home dir and he has run across a bug.

From email:

Hynek wrote:

I meant the loading resources for the UI (icons, bitmaps). I got your point about making the process more concise, but the think is it will degrade performance. So the question is whether we should descope this from #4065, which is primarily performance-focused.

Greg wrote:

The idea here is that we find them on the server side before we ever call down to the client side. It is not an upcall. We already do this in at least some cases, but it is not consistent with the rest of your code. Each place is just a patch done at some point rather than a designed approach.

Hynek wrote:

Can you give an example of the code that already does this?

See anything that calls LT.getResourceStreamFromApplication(String) or LT.getImageStreamFromApplication(String image).

The server-side shell execution will require more thought. For the file system it is straightforward to get a canonical path of the resource and check with the predefined ACLs. However for the shell command execution, resolving the canonical command (taking into account the environment, paths, etc.) is not trivial. The canonical representation of the executed command is necessary to avoid any ambiguities and avoid potential security holes.

In regard to the shell commands, I think it is OK if we require that the commands meet one of these criteria:

• It is an absolute path. Any such path can be canonicalized by our code and compared safely. OR
• It is a command with no pathing information. In such a case we would set a "secure" PATH from which that command can be safely found. That secure path would be configured in our directory.

Greg/Hynek: for server-side memptrs, we still have the problem when the memory address is incoming from a OS library call - as this is not tracked in the memptr$WorkArea.addressSpace (this contains only the addresses allocated via memptr.setLength).

We already have an application where server-side memptrs can't be used because of this limitation.

When a client process accesses the server, how do you know if its a local client (on the same host) or a remote one with this approach?

Take a look at the library manager support (or memptr) in trunk.

Thank you for the help. I'll find the configs now on trunk.

There is no email merge notification for task branch 4065 and no comment which revision it was on trunk. How do I find it?

Galya Bogdanova wrote:

There is no email merge notification for task branch 4065 and no comment which revision it was on trunk. How do I find it?

Please see #6075-25 for the main commit.

The initial support was put in as branch 3821c revision 13885/13886.

Additional support is written in 4065a but has not been merged to trunk yet as it needs review. But you can look at the existing code for the model.

Greg Shah wrote:

But you can look at the existing code for the model.

If I don't know the revision, how do I know what model we're speaking about.

Galya Bogdanova wrote:

Greg Shah wrote:

But you can look at the existing code for the model.

If I don't know the revision, how do I know what model we're speaking about.

When a 3821c revision is mentioned, that is a trunk revision. 3821c became trunk some time ago.

Constantin Asofiei wrote:

When a 3821c revision is mentioned, that is a trunk revision. 3821c became trunk some time ago.

So on trunk there is a model of something in a revision that was rebased, its number is lost. There is no wiki, but comments in a different task. I should start investigating from there.

Greg Shah wrote:

The initial support was put in as branch 3821c revision 13885/13886.

Ah, I see... it should be trunk 13885/13886.

Code Review Task Branch 4065a Revisions 14521 through 14527

Overall this is a really nice update! I think the approach is quite elegant. I'm happy that the implementation was done with a relatively small amount of change for the features provided.

1. BitFlagsResource.getRightsInstance() returns FileSystemRights instances instead of BitFlagsRights instances. This doesn't seem right.

2. In FileSystemDaemon.deleteWorker(), did you place the delete rights check after the recursion on purpose? Is the idea that the content may be deletable when the target is not deletable? My initial thought is that if you can't delete the target then you shouldn't have the chance to delete the contents, but I may be thinking incorrectly in this case. My sense is that the current result will be non-intuitive. In security-related matters, non-intuitive often means a security hole. If we're going to allow this we may want to check read access as well otherwise don't allow the recursion but I would go with just a top-level denial instead.

3. In regard to the StreamWrapper.assign() TODO, the protection code was put in with this commit:

revno: 11328.1.13
author: Hynek Cihlar <hc@goldencode.com>
committer: Constantin Asofiei <ca@goldencode.com>
branch nick: 4124a
timestamp: Tue 2019-07-16 13:37:01 +0200
message:
  Reworked the stream-by-name resolution. Now it all happens on the server, also the conversion changes were reverted.

  Refs #3751.

I don't know why it was done but I guess we have some case where we use something other than a RemoteStream on the server side and adding it is "no bueno". This deserves some additional consideration before merge.

4. GenericFrame.getRemoteStreamId() has dependencies on using RemoteStream. How do we handle that? The UI code really does need to stay on the client and redirected terminal stuff will need to be there too. If we have code that uses that, do we need some protection logic to ensure it fails safely when in server-side file system mode?

5. I prefer not to add dependencies in BitFlasgResource for edu.emory.mathcs.backport.java.util.*. It is not an approved 3rd party library.

6. DirStream should not have an import for org.apache.tinkerpop.gremlin.process.traversal.

7. In FileSystem, please add public to the new additions.

Constantin: Please review.

Greg Shah wrote:

Code Review Task Branch 4065a Revisions 14521 through 14527

Overall this is a really nice update! I think the approach is quite elegant. I'm happy that the implementation was done with a relatively small amount of change for the features provided.

1. BitFlagsResource.getRightsInstance() returns FileSystemRights instances instead of BitFlagsRights instances. This doesn't seem right.

True, this as unexpected change. I will fix this.

2. In FileSystemDaemon.deleteWorker(), did you place the delete rights check after the recursion on purpose? Is the idea that the content may be deletable when the target is not deletable? My initial thought is that if you can't delete the target then you shouldn't have the chance to delete the contents, but I may be thinking incorrectly in this case. My sense is that the current result will be non-intuitive. In security-related matters, non-intuitive often means a security hole. If we're going to allow this we may want to check read access as well otherwise don't allow the recursion but I would go with just a top-level denial instead.

Yes, the idea was "the content may be deletable while the target not". But I agree this is very counter-intuitive and will change this according your proposal - a top-level denial.

3. In regard to the StreamWrapper.assign() TODO, the protection code was put in with this commit:

[...]

I don't know why it was done but I guess we have some case where we use something other than a RemoteStream on the server side and adding it is "no bueno". This deserves some additional consideration before merge.

I'm not sure why I removed the check in the first place. I think this was needed at some point as the implementation evolved. I'll put the check back, the wrapper can handle remote and non-remote streams without problem.

4. GenericFrame.getRemoteStreamId() has dependencies on using RemoteStream. How do we handle that? The UI code really does need to stay on the client and redirected terminal stuff will need to be there too. If we have code that uses that, do we need some protection logic to ensure it fails safely when in server-side file system mode?

Since only file and dir streams are considered to be directly created on the server, anything else will work as ususal. This will include anything that needs to run on the client, like print and terminal streams.

5. I prefer not to add dependencies in BitFlasgResource for edu.emory.mathcs.backport.java.util.*. It is not an approved 3rd party library.

These were not needed, added by the IDE. Will remove.

6. DirStream should not have an import for org.apache.tinkerpop.gremlin.process.traversal.

Same as above.

7. In FileSystem, please add public to the new additions.

Will do.

• ConcurrentResource.resourceTypeName - the javadoc was indented right
• HighLevelObject.LOG - we are replacing java.util.logging in #5703, just something to keep track in #5703 or if #5703 gets merged before this
• LeafSessionManager.java, BitFlagsConstants.java, LogHelper.java - there is no change beside the end-of-file newline, this can be reverted
• ImportWorker has import com.goldencode.p2j.persist.pl.Functions - is this needed?
• BitFlagsResource - there is import edu.emory.mathcs.backport.java.util.* and import @edu.emory.mathcs.backport.java.util.Arrays;
• BitFlagsResource.isRightsSetValid - I don't understand this code:
  

         // check extended bits if any
           if (!set &&  extBits != null)
           {
              for (int i = 4; i < Math.min(3, extBits.length); i++)
              {
  

  
  i starts with 4 and goes to either 3 when extBits is greater or equal then 3, or something less than 3 otherwise. How is this loop supposed to work?
• DirStream has import org.apache.tinkerpop.gremlin.process.traversal.*
• FileSystemDaemon.getInstance is missing javadoc for isServerSideFS
• why do we need both filesystem and stream in OSResourceManager? Shouldn't this be an 'both or nothing'? Does this allow streams on server-side and file-system on client-side (or viceversa)?

Constantin Asofiei wrote:

Review for 4065a rev 14527:

• ConcurrentResource.resourceTypeName - the javadoc was indented right
• HighLevelObject.LOG - we are replacing java.util.logging in #5703, just something to keep track in #5703 or if #5703 gets merged before this
• LeafSessionManager.java, BitFlagsConstants.java, LogHelper.java - there is no change beside the end-of-file newline, this can be reverted
• ImportWorker has import com.goldencode.p2j.persist.pl.Functions - is this needed?
• BitFlagsResource - there is import edu.emory.mathcs.backport.java.util.* and import @edu.emory.mathcs.backport.java.util.Arrays;
• BitFlagsResource.isRightsSetValid - I don't understand this code:
  [...]
  i starts with 4 and goes to either 3 when extBits is greater or equal then 3, or something less than 3 otherwise. How is this loop supposed to work?
• DirStream has import org.apache.tinkerpop.gremlin.process.traversal.*
• FileSystemDaemon.getInstance is missing javadoc for isServerSideFS

I will address all the above.

• why do we need both filesystem and stream in OSResourceManager? Shouldn't this be an 'both or nothing'? Does this allow streams on server-side and file-system on client-side (or viceversa)?

Yes, currently you can configure the streams (currently file and dir streams) or file-system independently. I agree this could be merged together under a file-system resource. Greg, what do you think?

I agree this could be merged together under a file-system resource. Greg, what do you think?

Agreed.

I rebased 4065a against the latest trunk.

The points raised during the reviews are resolved in 4065a revision 14561. File history entries added in revision 14562. Please review.

Code Review Task branch 4065a Revisions 14560 through 14562

No objections. Weren't the logging changes removed on purpose to avoid conflicts with 5753d and 5703a?

Galya: Please review the logging changes in rev 14560.

Greg Shah wrote:

Galya: Please review the logging changes in rev 14560.

The changes are indeed unexpected.

Constantin Asofiei wrote:

Review for 4065a rev 14527:

• HighLevelObject.LOG - we are replacing java.util.logging in #5703, just something to keep track in #5703 or if #5703 gets merged before this

Also this is a surprising choice of logger: Logger.getLogger() - it doesn't use the standard LogHelper and will not cause compilation issue on rebase and might stay under the radar.

Greg Shah wrote:

Code Review Task branch 4065a Revisions 14560 through 14562

No objections. Weren't the logging changes removed on purpose to avoid conflicts with 5753d and 5703a?

I'm not aware of anybody removing them during trunk merge. At some point I manually added them to the branch and then removed. I assumed this has caused the removal during rebase.

Let's ensure that they don't cause conflicts with the upcoming merge of 5703a.

Also, we definitely should be using LogHelper.getLogger().

Hynek, I will obviously have to resolve the conflicts caused by all the changes you do to logging and then delete them. I hope that was clearly conveyed to you already. If you find it useful to have the changes on trunk for a few days, then I hope you enjoy them. :)

Greg Shah wrote:

Let's ensure that they don't cause conflicts with the upcoming merge of 5703a.

AFAIK Galya has successfully rebased with the logging changes. They are really minimal anyway and I don't think they should cause any troubles.

Also, we definitely should be using LogHelper.getLogger().

OK I will add it.

Hynek Cihlar wrote:

AFAIK Galya has successfully rebased with the logging changes. They are really minimal anyway and I don't think they should cause any troubles.

Sweet.

Galya Bogdanova wrote:

Hynek, I will obviously have to resolve the conflicts caused by all the changes you do to logging and then delete them. I hope that was clearly conveyed to you already. If you find it useful to have the changes on trunk for a few days, then I hope you enjoy them. :)

As long as the log format for the log entries stays consistent from the bootstrap on, then do whatever you like with my changes :-).

The change related to the reduction of package names seems to be useful.

Greg Shah wrote:

The change related to the reduction of package names seems to be useful.

Wasn't a requirement in #5703. Leave a note in #7279 for future Formatter changes.

Logger.getLogger replaced with LogHelper in 4065a revision 14563. Galya, please review.

Hynek Cihlar wrote:

Logger.getLogger replaced with LogHelper in 4065a revision 14563. Galya, please review.

There is nothing to review here.

I'm still not clear though if this is an acceptable working process - create an issue yourself, ask about permission to solve, don't receive it, understand it causes conflicts with an ongoing complete revamp of the same feature, then solve it in your own way in an unrelated task without notifying anyone about it explicitly and merge it to trunk, repeat last two twice? I mean can I do it as well, Greg? :)

Galya Bogdanova wrote:

Hynek Cihlar wrote:

Logger.getLogger replaced with LogHelper in 4065a revision 14563. Galya, please review.

There is nothing to review here.

I'm still not clear though if this is an acceptable working process - create an issue yourself, ask about permission to solve, don't receive it, understand it causes conflicts with an ongoing complete revamp of the same feature, then solve it in your own way in an unrelated task without notifying anyone about it explicitly and merge it to trunk, repeat last two twice? I mean can I do it as well, Greg? :)

If it's a tiny change fixing a bug and then the change is reviewed then I don't see any issue with that.

We could argue about the formatter change. But knowing the product and the customers for so long I was sure this would be a welcome change.

In any case I didn't want to sneak in any changes in. Everything was reviewed as was any other task branch.

The changes are so minimal that I don't see how it can cause issues during merging with your work. And I offered you help with any potential conflicts.

Hynek Cihlar wrote:

The changes are so minimal that I don't see how it can cause issues during merging with your work.

I'm not sure if you understand that the classes don't exist any more, so your changes are wiped out, if you don't request me to change my implementation in the last minute. That's why it's rude to pretend you don't hear, when you're warned someone does a refactoring on the same feature and don't care at all what it is. If you're so opinionated, so well familiar with the client and you don't want to discuss it, then I guess Greg had to give you the logging tasks. I'm completely fine to give you #5703 to take over it. Just ask the team leads to assign it to you.

Galya Bogdanova wrote:

Hynek Cihlar wrote:

The changes are so minimal that I don't see how it can cause issues during merging with your work.

I'm not sure if you understand that the classes don't exist any more, so your changes are wiped out, if you don't request me to change my implementation in the last minute.

The code doesn't matter. It's important so that we keep the product good. If it means you need to wipe out my changes, let's be it.

Hynek Cihlar wrote:

The code doesn't matter. It's important so that we keep the product good. If it means you need to wipe out my changes, let's be it.

Requirements go into related tasks, then get discussed and the best solution is taken. If you sneak changes in, they will get wiped out, not because it makes the product worse, but because of lack of proper communication and a fault in the work process created by your eagerness for improvement. I haven't seen an application code in a state of art, that doesn't need improvements, so you need to doubt your ideal solutions to make something perfect.

For example your vision of the product may have a flaw:
Hynek Cihlar wrote:

But knowing the product and the customers for so long I was sure this would be a welcome change.

I thought we're doing an open source framework, not a product for one specific customer.

It is not uncommon for us to implement small changes to unrelated areas in tasks that are otherwise addressing a different feature/purpose.

Since we do not use a traditional waterfall process, our design work tends to be done in stages during our tasks rather than all at once. This means we:

• Can start work faster and deliver in more incremental steps.
• Work more independently and asynchronously.
• Decentralize our design.
• Require more discussions, across more tasks.
• Can have changes later that were not foreseen at the beginning.
• Will have the work on specific features split over many tasks and many team members.

These last two items can be frustrating when you are working on a big update, but it is a consequence of our process. No one here is trying to make life harder for their teammates.

But knowing the product and the customers for so long I was sure this would be a welcome change.

I thought we're doing an open source framework, not a product for one specific customer.

We take feedback and requirements from our multiple existing customers into account when implementing changes that everyone will see. This is a good thing.

Hynek is correct that our customers have told us on many occasions that the stack traces are overwhelming. Reducing the text representation of the package names is a good enhancement, which is why we want to keep it.

If you're so opinionated, so well familiar with the client and you don't want to discuss it, then I guess Greg had to give you the logging tasks. I'm completely fine to give you #5703 to take over it. Just ask the team leads to assign it to you.

I would hope that we can integrate these changes together without moving all logging work to Hynek.

Greg Shah wrote:

I would hope that we can integrate these changes together without moving all logging work to Hynek.

Of course, when the requirements are defined in a task. I'll be waiting for it.

I think this discussion qualifies as defining the requirements for the "reduce package name text in stack trace output" feature. Hynek has written the changes and tested that they work. Rather than defer this to a separate work stream, let's discuss how his work can be used.

Please remove all the logging changes from rev 14563. Attach a patch for the package name abbreviation code to #7279. We will apply it to that task's branch when there is one. The other changes are not needed, as far as I know. This will eliminate any conflict with 5703a, which has 500+ files edited so we do want to make the 5703a rebase and merge process easier.

After 4065a is merged to trunk the following resources still need a server-side implementation:

• process launching
• XML
• web-services
• server sockets

Greg Shah wrote:

Please remove all the logging changes from rev 14563. Attach a patch for the package name abbreviation code to #7279.

OK, no problem. Just please note this will cause another round of code conflicts for Galya.

I reverted the log changes introduced in trunk revision 14535 in 4065a revision 14565.

Please merge 4065a to trunk.

4065a merged in to trunk revision 14554 and archived.

Constantin, what is currently the least stressful way to test web-services?

The wiki is here: Running Web Tests. You will need SoapUI, and possible need to edit the host and port.

XML load/save is already handled with the server-side file access. It is enough to activate server-side file resource.

Constantin Asofiei wrote:

The wiki is here: Running Web Tests. You will need SoapUI, and possible need to edit the host and port.

Converting Web tests gives the following errors. Any idea?

    [java] Null annotation (full-java-class) for oo.serialization.SerializeAll [CLASS_NAME] @1:47 (468151435277)
     [java] Null annotation (simple-java-class) for oo.serialization.SerializeAll [CLASS_NAME] @1:47 (468151435277)
     [java] Null annotation (containing-package) for oo.serialization.SerializeAll [CLASS_NAME] @1:47 (468151435277)
     [java] Null annotation (full-java-class) for define [DEFINE_PARAMETER] @1:1 (468151435267)
     [java] Null annotation (simple-java-class) for define [DEFINE_PARAMETER] @1:1 (468151435267)
     [java] Null annotation (containing-package) for define [DEFINE_PARAMETER] @1:1 (468151435267)
     [java] Null annotation (full-java-class) for oo.serialization.SerializeAll [CLASS_NAME] @2:46 (468151435291)
     [java] Null annotation (simple-java-class) for oo.serialization.SerializeAll [CLASS_NAME] @2:46 (468151435291)
     [java] Null annotation (containing-package) for oo.serialization.SerializeAll [CLASS_NAME] @2:46 (468151435291)
     [java] Null annotation (full-java-class) for define [DEFINE_PARAMETER] @2:1 (468151435281)
     [java] Null annotation (simple-java-class) for define [DEFINE_PARAMETER] @2:1 (468151435281)
     [java] Null annotation (containing-package) for define [DEFINE_PARAMETER] @2:1 (468151435281)
     [java] Null annotation (full-java-class) for serializedObject [VAR_CLASS] @5:1 (468151435298)
     [java] Null annotation (simple-java-class) for serializedObject [VAR_CLASS] @5:1 (468151435298)
     [java] Null annotation (containing-package) for serializedObject [VAR_CLASS] @5:1 (468151435298)
     [java] Null annotation (full-java-class) for objectToSerialize [VAR_CLASS] @5:20 (468151435300)
     [java] Null annotation (simple-java-class) for objectToSerialize [VAR_CLASS] @5:20 (468151435300)
     [java] Null annotation (containing-package) for objectToSerialize [VAR_CLASS] @5:20 (468151435300)
     [java] EXPRESSION EXECUTION ERROR:
     [java] ---------------------------
     [java] persist()
     [java] ^  { null value for annotation 'full-java-class':define [DEFINE_PARAMETER]:468151435267 @1:1
     [java]    input [KW_INPUT]:468151435269 @1:9
     [java]    objectToSerialize [SYMBOL]:468151435273 @1:26
     [java]    as [KW_AS]:468151435275 @1:44
     [java]       oo.serialization.SerializeAll [CLASS_NAME]:468151435277 @1:47
     [java]  }
     [java] ---------------------------
     [java] ERROR:
     [java] com.goldencode.p2j.pattern.TreeWalkException: ERROR!  Active Rule:
     [java] -----------------------
     [java]       RULE REPORT      
     [java] -----------------------
     [java] Rule Type :   POST
     [java] Source AST:  [ block ] BLOCK/ @0:0 {468151435265}
     [java] Copy AST  :  [ block ] BLOCK/ @0:0 {468151435265}
     [java] Condition :  persist()
     [java] Loop      :  false
     [java] --- END RULE REPORT ---
     [java] 
     [java] 
     [java] 
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1099)
     [java]     at com.goldencode.p2j.convert.TransformDriver.processTrees(TransformDriver.java:587)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.back(ConversionDriver.java:563)
     [java]     at com.goldencode.p2j.convert.TransformDriver.executeJob(TransformDriver.java:999)
     [java]     at com.goldencode.p2j.convert.ConversionDriver.main(ConversionDriver.java:1284)
     [java] Caused by: com.goldencode.expr.ExpressionException: Expression execution error @1:1
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:495)
     [java]     at com.goldencode.p2j.pattern.Rule.apply(Rule.java:500)
     [java]     at com.goldencode.p2j.pattern.RuleContainer.apply(RuleContainer.java:590)
     [java]     at com.goldencode.p2j.pattern.RuleSet.apply(RuleSet.java:98)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.apply(PatternEngine.java:1710)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1577)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.processAst(PatternEngine.java:1510)
     [java]     at com.goldencode.p2j.pattern.PatternEngine.run(PatternEngine.java:1062)
     [java]     ... 4 more
     [java] Caused by: java.lang.NullPointerException: null value for annotation 'full-java-class':define [DEFINE_PARAMETER]:468151435267 @1:1
     [java]    input [KW_INPUT]:468151435269 @1:9
     [java]    objectToSerialize [SYMBOL]:468151435273 @1:26
     [java]    as [KW_AS]:468151435275 @1:44
     [java]       oo.serialization.SerializeAll [CLASS_NAME]:468151435277 @1:47
     [java] 
     [java]     at com.goldencode.ast.XmlFilePlugin.writeSingleAnnotation(XmlFilePlugin.java:1154)
     [java]     at com.goldencode.ast.XmlFilePlugin.writeAnnotations(XmlFilePlugin.java:1115)
     [java]     at com.goldencode.ast.XmlFilePlugin.writeAst(XmlFilePlugin.java:1074)
     [java]     at com.goldencode.ast.XmlFilePlugin.writeAst(XmlFilePlugin.java:1080)
     [java]     at com.goldencode.ast.XmlFilePlugin.writeAst(XmlFilePlugin.java:1080)
     [java]     at com.goldencode.ast.XmlFilePlugin.saveTree(XmlFilePlugin.java:518)
     [java]     at com.goldencode.ast.AstManager.saveTree(AstManager.java:360)
     [java]     at com.goldencode.p2j.pattern.CommonAstSupport$Library.persist(CommonAstSupport.java:2972)
     [java]     at com.goldencode.expr.CE10873.execute(Unknown Source)
     [java]     at com.goldencode.expr.Expression.execute(Expression.java:398)
     [java]     ... 11 more

Is oo/serialization/SerializeAll.cls included in the conversion list? All referenced OO classes/interfaces/enums in the entire object graph must be included in the conversion list.

Greg Shah wrote:

Is oo/serialization/SerializeAll.cls included in the conversion list? All referenced OO classes/interfaces/enums in the entire object graph must be included in the conversion list.

The class file is not in the conversion list, but is properly resolved and converted. Adding it to the conversion list indeed resolves the error. Does it really need to be in the list?

Yes, until #6082 is complete.

Greg, is the scope of the server-side web-services only the invocation part (i.e. WebServiceHelper, WebServiceImpl) or also the serving part (i.e. WebServiceHandler)?

It is the client side (calling) only.

I created task branch 4065b and checked in a fix for the regression reported in #5731-856.

ThinClient must be instantiated yet before the custom authentication hook is invoked.

Please review.

Hynek, I think there is some confusion: REST/SOAP/WebHandler are for the FWD web service support (server-side deployment). I think you mean the socket/server-socket, and SOAP clients - these are used by 4GL converted code, where currently the 4GL socket is being opened and managed on client-side.

I thought we already supported regular 4GL client sockets on the server side, but the missing part was the web services client calls.

The 4GL server sockets would be an additional item to move over.

Greg Shah wrote:

I thought we already supported regular 4GL client sockets on the server side,

Yes, 4GL client sockets are implemented.

but the missing part was the web services client calls.

Correct, web service client calls are implemented in p2j.util.WebServiceImpl.

The 4GL server sockets would be an additional item to move over.

Correct.

• for server-sockets, use the old testcases project, uast/sockets/socket-run.p - there are socket-client.p and socket-server.p programs which are ran by this.
• for web services (SOAP), Marian added tests in xfersrv01:testcases, but I haven't ran those - they rely on the FWD SOAP web service support (what I mentioned earlier), plus some 4GL testcases which connect to these services ran in FWD. There are also tests in uast/web_services, with a server/ folder, but I never ran this. Originally I used a 3rd-party web service URL, but that is down now - see uast/web_services/wsdl_test1.p.

I've found another free 3rd-party web service - see this simple test:

def var h as handle.
def var l as log.
def var hp as handle.
def var chi as char.
def var cho as char.

create server h.

l = h:connect("-WSDL http://webservices.oorsprong.org/websamples.countryinfo/CountryInfoService.wso?WSDL  -nohostverify  -Service CountryInfoService").
if l <> yes then do: message "could not connect". pause. quit. end.

run CountryInfoServiceSoapType set hp on server h.

if not valid-handle(hp) then do: message "could not run port". pause. quit. end.

chi = 'RO'.

output to log.txt.
run LanguageName in hp(input chi, output cho).

message cho.
output close.

Greg, also, something else to note: there is code in p2j.oo for client sockets and also HTTP connections. Marian, please remind me which p2j.oo classes are the implementation of the socket or HTTP call.

Constantin Asofiei wrote:

Greg, also, something else to note: there is code in p2j.oo for client sockets and also HTTP connections. Marian, please remind me which p2j.oo classes are the implementation of the socket or HTTP call.

The 'client socket' OO implementation is in oo.net.serverconnection.ClientSocket but the implementation is mostly only stub. The HTTP client uses the library from oo.net.http.lib.sockets. LegacySocketLibrary but that one is not using the 'client socket' as in 4GL but the apache http client.

Hynek Cihlar wrote:

I created task branch 4065b and checked in a fix for the regression reported in #5731-856.

ThinClient must be instantiated yet before the custom authentication hook is invoked.

Please review.

This addresses the regression mentioned in #5731-856.

Code Review Task Branch 4065b Revision 14571

No objections.

4065b revision 14576 implements server-side web service calling. Please review.

I named the "resource" just "webservice". It would probably make sense to rename it to webservice-client and distinguish it from the web service handler.

I checked in a fix for another client initialization problem in 4065b revision 14577. StreamDaemon initialization must be postponed after establishing user session. Please review.

Code Review Task Branch 4065b Revisions 14576 and 14577

1. Doesn't ClientSideResourceManager.initialize() need a new entry for WEBSERVICE_RESOURCE?

2. OSResourceManager.isServerSideWebService() should reference WEBSERVICE_RESOURCE instead of FILESYSTEM_RESOURCE.

3. Shouldn't we still call WebServiceImpl.initialize() (even though it is empty) so that if someone puts logic in there later, it will work?

Greg Shah wrote:

Code Review Task Branch 4065b Revisions 14576 and 14577

1. Doesn't ClientSideResourceManager.initialize() need a new entry for WEBSERVICE_RESOURCE?

2. OSResourceManager.isServerSideWebService() should reference WEBSERVICE_RESOURCE instead of FILESYSTEM_RESOURCE.

The above resolved im 4065b revision 14578.

3. Shouldn't we still call WebServiceImpl.initialize() (even though it is empty) so that if someone puts logic in there later, it will work?

The method is a SessionListener override and gets called by BaseSession.

I named the "resource" just "webservice". It would probably make sense to rename it to webservice-client and distinguish it from the web service handler.

Agreed. Perhaps the socket usage should also be noted as the socket-client version?

The method is a SessionListener override and gets called by BaseSession.

OK.

Are you working on process launching and server-sockets?

Greg Shah wrote:

Are you working on process launching and server-sockets?

No, I switched to my other priority tasks.

Are you working on process launching and server-sockets?

No, I switched to my other priority tasks.

Let's get this task complete first. I think when these two are added, we can close the task.

Greg, I'd like to merge 4065b to trunk to address the regressions.

Go ahead.

Greg Shah wrote:

Go ahead.

In progress.

4065b merged to trunk as revision 14580 and archived.

Hynek, there is a regression in trunk rev 14580 - this seems to fix it, I need to finish ETF testing to confirm.

=== modified file 'src/com/goldencode/p2j/ui/chui/ThinClient.java'
--- old/src/com/goldencode/p2j/ui/chui/ThinClient.java  2023-05-22 09:01:31 +0000
+++ new/src/com/goldencode/p2j/ui/chui/ThinClient.java  2023-05-24 16:07:55 +0000
@@ -3498,7 +3498,6 @@
       sigintThread = new SigintWaiter(this);

       new ComOleDaemon(single);
-      pd   = new ProcessDaemon(tk.getInstanceDriver().getChildProcessFactory(), single, sd);
       ospd = new OsPropertiesDaemon(single);

       // load the FWD native libraries
@@ -3677,6 +3676,7 @@
    {
       fileSystem = OSResourceManager.getInstance().initializeFileSystem();
       sd = new StreamDaemon(this, single);
+      pd = new ProcessDaemon(tk.getInstanceDriver().getChildProcessFactory(), single, sd);
    }

    /**

Constantin Asofiei wrote:

Hynek, there is a regression in trunk rev 14580 - this seems to fix it, I need to finish ETF testing to confirm.

This patch fixes the problem in my testing.

Constantin Asofiei wrote:

Constantin Asofiei wrote:

Hynek, there is a regression in trunk rev 14580 - this seems to fix it, I need to finish ETF testing to confirm.

This patch fixes the problem in my testing.

Good, it looks OK to me.

I implemented server sockets to be a distinct resource from client sockets. So currently one can define server sockets to be server-side while client-sockets to execute on the client. Now server side sockets use the client sockets for the accepted connections, and so the configuration may lead to listening socket running on the server but connection socket on the client. This is confusing and I can't think of a use case where this would be desired. Thus I will move the server and client sockets under single resource "socket".

For the socket client the key store password is read from the client parameters ssl-socket:truststore:password. However for the listening socket the password is read from ssl-socket:keystore:password. This doesn't seem right. Should the password be read from the same location for both?

Hynek Cihlar wrote:

For the socket client the key store password is read from the client parameters ssl-socket:truststore:password. However for the listening socket the password is read from ssl-socket:keystore:password. This doesn't seem right. Should the password be read from the same location for both?

OK, according to Certificates_and_Keys_for_4GL_Language_Features it is correct.

Greg, for the process resource, launch will be conducted on the server only when no terminal is needed. That is for the OS-COMMAND with NO-CONSOLE and NO-WAIT, and for INPUT/OUTPUT/INPUT-OUTPUT THROUGH with file-based streams. For the other cases launch will be passed to the client as usual.

I implemented server sockets to be a distinct resource from client sockets. So currently one can define server sockets to be server-side while client-sockets to execute on the client.

This is optimal. It is possible that either one might need to be split out to the actual client while the other can be safely used on the server.

Now server side sockets use the client sockets for the accepted connections, and so the configuration may lead to listening socket running on the server but connection socket on the client. This is confusing and I can't think of a use case where this would be desired.

I agree this is not useful.

Thus I will move the server and client sockets under single resource "socket".

What is the effort to decouple these so that the server's inbound connections do not use the client socket resources?

Greg, for the process resource, launch will be conducted on the server only when no terminal is needed. That is for the OS-COMMAND with NO-CONSOLE and NO-WAIT, and for INPUT/OUTPUT/INPUT-OUTPUT THROUGH with file-based streams. For the other cases launch will be passed to the client as usual.

Understood. For the terminal cases, even though we could use ncurses (via libp2j.so in the server), we would have to move some substantial part of the UI processing to the server. Another reason is that our native terminal processing is probably not necessarily thread safe. And I'm unsure if the pseudoterminal support in Linux has any limitations on threading, we would have to investigate that aspect.

Greg Shah wrote:

What is the effort to decouple these so that the server's inbound connections do not use the client socket resources?

This shouldn't take more than 1 MD.

Hynek Cihlar wrote:

Greg Shah wrote:

What is the effort to decouple these so that the server's inbound connections do not use the client socket resources?

This shouldn't take more than 1 MD.

Please go ahead with it so that we can keep two independent resources (socket-server and socket-client).

Greg Shah wrote:

Hynek Cihlar wrote:

Greg Shah wrote:

What is the effort to decouple these so that the server's inbound connections do not use the client socket resources?

This shouldn't take more than 1 MD.

Please go ahead with it so that we can keep two independent resources (socket-server and socket-client).

OK

Is the fix for #4065-150 in a branch?

Greg Shah wrote:

Is the fix for #4065-150 in a branch?

Not yet AFAIK. I will create task branch 4065d for this. 4065c already contains changes for the listening socket and process.

The regression fix in #4065-150 was checked in 4065d and is ready to be merged to trunk once it passes review.

Code Review Task Branch 4065d Revision 14593

No objections. You can merge to trunk now.

4065d rebased and merged to trunk as revision 14594. The branch was archived.

This feature is not properly documented yet. As noted in #6075-25 and #4065-18, it can configured with a server-side-resources node like this:

                <node class="string" name="server-side-resources">
                  <node-attribute name="value" value="memptr,library"/>
                </node>

The cfg reading can be seen in the constructor for OSResourceManager. The values that can be used in the comma-separated string are:

• memptr
• library
• socket
• filesystem
• webservice

There is also the special all value which enables server-side processing for all supported resources. At this time, server-side logging is configured and implemented separately.

As with all of our context-specific 4GL runtime options, this can be set at global default, server default, server, group/account levels. A simple test with a global setting would place this in /server/default/runtime/default/.

This configuration is not enough. For example, to enable server-side filesystem, you also need:

<node class="strings" name="resource-plugins">
  <node-attribute name="values" value="com.goldencode.p2j.security.FileSystemResource"/>
</node>

And this on its turn is still not enough, because read and write permissions are still missing for the user. I can't remember how it should be configured.

Galya B wrote:

This configuration is not enough. For example, to enable server-side filesystem, you also need:

[...]

And this on its turn is still not enough, because read and write permissions are still missing for the user. I can't remember how it should be configured.

You typically configure the ACLs in FWD Admin.

Here is an example of a file-system ACL entry:

        <node class="container" name="file-system">
          <node class="container" name="005500">
            <node class="resource" name="resource-instance">
              <node-attribute name="reference" value="ls"/>
              <node-attribute name="reftype" value="TRUE"/>
            </node>
            <node class="fileSystemRights" name="rights">
              <node-attribute name="permissions" value="'00010000'B"/>
            </node>
            <node class="strings" name="subjects">
              <node-attribute name="values" value="all_others"/>
            </node>
          </node>
       </node>

Where the reference points to a file system object (ls command in this case) and the permissions attribute contains the assigned permissions. For their values look in FileSystemRights and BitFlagsConstants Java classes.