Page MenuHomePhabricator

ecore_ipc: restore ability for negative port on local/remote ipc connection
AbandonedPublic

Authored by zmike on Feb 4 2018, 11:33 PM.

Details

Summary

This patch restores ability for negative port on local/remote ipc connection like ecore_con_server_add by checking valid port number.

Test Plan

Execute test suite

Diff Detail

Repository
rEFL core/efl
Branch
fix_work
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5737
Build 6370: arc lint + arc unit
myoungwoon requested review of this revision.Feb 4 2018, 11:33 PM
myoungwoon created this revision.
cedric added a comment.Feb 5 2018, 9:58 AM

Actually do we need to do any check here because ecore_con does the check for us, no ?

src/lib/ecore_ipc/ecore_ipc.c
398

Isn't REMOTE_SYSTEM the only case when we really want to limit port to between 0 and 65535 ?

myoungwoon added a comment.EditedFeb 5 2018, 5:55 PM
In D5789#98982, @cedric wrote:

Actually do we need to do any check here because ecore_con does the check for us, no ?

As you know, ecore_ipc_server_add is a public API on upstream and Tizen. It means user call this API any time in order to create an IPC server that listens for connections.
And generally IPC uses files, pipes, signals, shared memory, and scockets based on UDP/TCP. So I think the port number should be checked for socket programming like ecore_con_server_add in case of using sockets basis communication.

Actually, port already had been checked from following code.
else if ((type & ECORE_IPC_TYPE) == ECORE_IPC_REMOTE_SYSTEM)

{
   char buf[4096];

   if (port <= 0)
     {

I think this checking should be front for other ECORE_IPC types because other types also use port number which is greater than zero and smaller than 65535(16-bit identifier).

If only port number is using for REMOTE_SYSTEM, namely, only ECORE_IPC_REMOTE_SYSTEM uses port number, then you're right. my checking should be moved to REMOTE_SYSTEM condition statement.

raster added a comment.Feb 7 2018, 2:52 AM

cedric's point is that ecore_ipc just sits on top of ecore_con and if ecore_con does these checks already... it will fail there and ecore_ipc will fail to connect or seet up a server, so there is no need to check twice.

myoungwoon updated this revision to Diff 13822.Feb 7 2018, 5:39 PM

Port is an abstraction for socket ipc communication.
If port is not NULL when calling ecore_ipc_server_add, it means users try to use sockets with port.
If users use sockets with port, port number should be checked.
Zero port number is no issue because it means select any available local port.

myoungwoon updated this revision to Diff 13823.Feb 7 2018, 6:03 PM

update comments.

myoungwoon added a comment.EditedFeb 7 2018, 6:22 PM
In D5789#99069, @raster wrote:

cedric's point is that ecore_ipc just sits on top of ecore_con and if ecore_con does these checks already... it will fail there and ecore_ipc will fail to connect or seet up a server, so there is no need to check twice.

Generally, other IPC such as shared memory, pipes, signals should be used only locally. and Sockets should be used for both local or remote.
As you mentioned, incase of ECORE_IPC_LOCAL_USER or ECORE_IPC_LOCAL_SYSTE, ecore_con_local_path_new function checks port number.
However, it is just for creating an address for other IPC and socket based IPC with port number without checking the validation of port number.
As a result, when user try to use a socket as local IPC with a invalid port number, there is no notification about invalid binding(connection) on sockets during executing efl_net_server_serve().
Therefore, IMO, if port number is not null, port number should be checked front because users use socket for IPC(both local and remote).

As a result, when user try to use a socket as local IPC with a invalid port number, there is no notification about invalid binding(connection) on sockets during executing efl_net_server_serve().

That sounds like a bug in efl_net_server_serve(), no ? It is supposed to return an error if it fail. If this is not working appropriately, shouldn't that be fixed there ?

yeah. cedric is right. try this with an efl/.net example with negative ports... you should end up getting a failure event. look at efl_net_server_example.c for example for creation of a server. efl_net_server_serve() indicates how to listen to the service... so "0.0.0.0:-1" should do it as the address.

zmike commandeered this revision.May 2 2018, 3:00 PM
zmike added a reviewer: myoungwoon.
zmike added a subscriber: zmike.

This patch seems a bit misguided; I've spent some time reviewing existing code and doing some tests, and the ecore-con layer absolutely does catch errors. What exactly constitutes an error:

  • using a remote socket with an invalid port number
  • nothing else

Any port number can be used with a local socket since the 'port number' is just added to the end of the socket filename as a string and has no relation to any port numbering limitations.

zmike abandoned this revision.May 2 2018, 3:00 PM