Nsock IOCP engine review

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Nsock IOCP engine review

Henri Doreau
Hello,

I just reviewed the new nsock IOCP engine. I unfortunately could not
test it but I have no doubt that others will seriously stress it.
Congratulations for the good work. I have a couple concerns though.
Two of which are minor, one's slightly more annoying.

Starting with the small ones:

* there seems to be engine-specific data embedded directly into the
nsp. That doesn't make sense to me... Why not having them in the
engine_data which you use anyway?

* in nbase_misc.c: is it ok to unconditionally specify the
WSA_FLAG_OVERLAPPED? What if another engine is selected?


Now, for something bigger:
* the following is everywhere, and is an anti-pattern, something that
the nsock engines are supposed to guard us against:
"""
#ifdef HAVE_IOCP
if (engine_is_iocp(nsp))
    iocp_specific_call()
#endif
"""

This makes the code difficult to review/understand/test/maintain. The
function was even checked without being called at some point (removed
as of r35969). I regard this as a proof that such a pattern is very
error-prone.

Can you consider moving these blocks to engine-specific functions?
Maybe they would fit somewhere in the existing functions, or maybe the
interface has to evolve. If so, find the appropriate abstractions, add
corresponding calls to the engine operations and implement them as
such (they can be left empty for the other engines if irrelevant, no
problem with that of course)...

I would have nacked the change if asked. Now I'm fine with not
reverting r36093 if you can address the issues above. Otherwise I
consider that it introduces a significant technical debt, despite how
nice the feature is! It's good work, but not ready to land IMHO.

Tudor, Fyodor, Dan? What do you think?

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Fw: Nsock IOCP engine review

Tudor-Emil COMAN





From: Tudor-Emil COMAN
Sent: Wednesday, August 10, 2016 6:24 PM
To: Henri Doreau
Subject: Re: Nsock IOCP engine review
 

Hi,


>* in nbase_misc.c: is it ok to unconditionally specify the
>WSA_FLAG_OVERLAPPED? What if another engine is selected?


This only affects nmap on Windows. 

This is just to enable overlapped operations on the socket.

This doesn't affect the behavior of sockets for non-overlapped operations.

Even the windows documentation suggests that "Most sockets should be created with this flag set."

https://msdn.microsoft.com/en-us/library/windows/desktop/ms742212(v=vs.85).aspx



>* there seems to be engine-specific data embedded directly into the
>nsp. That doesn't make sense to me... Why not having them in the
>engine_data which you use anyway?


Using the engine_data outside iocp_engine.c was a new development. But you are right, we should move stuff from nsp there.

This should be doable.



>Now, for something bigger:
>* the following is everywhere, and is an anti-pattern, something that
>the nsock engines are supposed to guard us against:
>"""
>#ifdef HAVE_IOCP
>if (engine_is_iocp(nsp))
>    iocp_specific_call()
>#endif
>"""


This is what I've been afraid of. The fact that IOCP is so different from other engines means it should do some things differently.

I tried conforming to the API but if we can add some minor changes I'm 90% sure we can find a solution to this.

We could add to each engine their own read, write, connect function in *_engine.c.

Also each engine would need to have an initiate_event and terminate_event. These would be empty for every other engine except engine_iocp.


Thoughts?


Thanks for the review,

Tudor


From: Henri Doreau <[hidden email]>
Sent: Wednesday, August 10, 2016 5:05:42 PM
To: Tudor-Emil COMAN; Fyodor; Daniel Miller; Nmap dev
Subject: Nsock IOCP engine review
 
Hello,

I just reviewed the new nsock IOCP engine. I unfortunately could not
test it but I have no doubt that others will seriously stress it.
Congratulations for the good work. I have a couple concerns though.
Two of which are minor, one's slightly more annoying.

Starting with the small ones:

* there seems to be engine-specific data embedded directly into the
nsp. That doesn't make sense to me... Why not having them in the
engine_data which you use anyway?

* in nbase_misc.c: is it ok to unconditionally specify the
WSA_FLAG_OVERLAPPED? What if another engine is selected?


Now, for something bigger:
* the following is everywhere, and is an anti-pattern, something that
the nsock engines are supposed to guard us against:
"""
#ifdef HAVE_IOCP
if (engine_is_iocp(nsp))
    iocp_specific_call()
#endif
"""

This makes the code difficult to review/understand/test/maintain. The
function was even checked without being called at some point (removed
as of r35969). I regard this as a proof that such a pattern is very
error-prone.

Can you consider moving these blocks to engine-specific functions?
Maybe they would fit somewhere in the existing functions, or maybe the
interface has to evolve. If so, find the appropriate abstractions, add
corresponding calls to the engine operations and implement them as
such (they can be left empty for the other engines if irrelevant, no
problem with that of course)...

I would have nacked the change if asked. Now I'm fine with not
reverting r36093 if you can address the issues above. Otherwise I
consider that it introduces a significant technical debt, despite how
nice the feature is! It's good work, but not ready to land IMHO.

Tudor, Fyodor, Dan? What do you think?

--
Henri

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: Nsock IOCP engine review

Tudor-Emil COMAN

To eliminate that anti-pattern we would need to add the following functions to io_engine.


int(*event_initiate)(struct npool *nsp, struct nevent *nse);
int(*event_terminate)(struct npool *nsp, struct nevent *nse);
int(*iod_connect)(struct npool *nsp, int sockfd, const struct sockaddr *addr, socklen_t addrlen);
int(*iod_read)(struct npool *nsp, int sockfd, void *buf, size_t len, int flags,
                      struct sockaddr *src_addr, socklen_t *addrlen);
int(*iod_write)(struct npool *nsp, int sockfd, const void *buf, size_t len, int flags,
                       const struct sockaddr *dest_addr, socklen_t addrlen);

For any other engine except iocp:
  - event_initiate and event_terminate are empty
  - iod_connect, iod_read, iod_write are wrappers over connect, sendto, recvfrom

I took all the code from nsock_iocp.c and added it to engine_iocp.c.
Everything I added to struct npool can be moved to iocp_engine_info.

With these we can contain everything iocp related to engine_iocp.c.

Are these changes acceptable?

Cheers,
Tudor


From: dev <[hidden email]> on behalf of Tudor-Emil COMAN <[hidden email]>
Sent: Wednesday, August 10, 2016 8:20:38 PM
To: [hidden email]
Subject: Fw: Nsock IOCP engine review
 





From: Tudor-Emil COMAN
Sent: Wednesday, August 10, 2016 6:24 PM
To: Henri Doreau
Subject: Re: Nsock IOCP engine review
 

Hi,


>* in nbase_misc.c: is it ok to unconditionally specify the
>WSA_FLAG_OVERLAPPED? What if another engine is selected?


This only affects nmap on Windows. 

This is just to enable overlapped operations on the socket.

This doesn't affect the behavior of sockets for non-overlapped operations.

Even the windows documentation suggests that "Most sockets should be created with this flag set."

https://msdn.microsoft.com/en-us/library/windows/desktop/ms742212(v=vs.85).aspx



>* there seems to be engine-specific data embedded directly into the
>nsp. That doesn't make sense to me... Why not having them in the
>engine_data which you use anyway?


Using the engine_data outside iocp_engine.c was a new development. But you are right, we should move stuff from nsp there.

This should be doable.



>Now, for something bigger:
>* the following is everywhere, and is an anti-pattern, something that
>the nsock engines are supposed to guard us against:
>"""
>#ifdef HAVE_IOCP
>if (engine_is_iocp(nsp))
>    iocp_specific_call()
>#endif
>"""


This is what I've been afraid of. The fact that IOCP is so different from other engines means it should do some things differently.

I tried conforming to the API but if we can add some minor changes I'm 90% sure we can find a solution to this.

We could add to each engine their own read, write, connect function in *_engine.c.

Also each engine would need to have an initiate_event and terminate_event. These would be empty for every other engine except engine_iocp.


Thoughts?


Thanks for the review,

Tudor


From: Henri Doreau <[hidden email]>
Sent: Wednesday, August 10, 2016 5:05:42 PM
To: Tudor-Emil COMAN; Fyodor; Daniel Miller; Nmap dev
Subject: Nsock IOCP engine review
 
Hello,

I just reviewed the new nsock IOCP engine. I unfortunately could not
test it but I have no doubt that others will seriously stress it.
Congratulations for the good work. I have a couple concerns though.
Two of which are minor, one's slightly more annoying.

Starting with the small ones:

* there seems to be engine-specific data embedded directly into the
nsp. That doesn't make sense to me... Why not having them in the
engine_data which you use anyway?

* in nbase_misc.c: is it ok to unconditionally specify the
WSA_FLAG_OVERLAPPED? What if another engine is selected?


Now, for something bigger:
* the following is everywhere, and is an anti-pattern, something that
the nsock engines are supposed to guard us against:
"""
#ifdef HAVE_IOCP
if (engine_is_iocp(nsp))
    iocp_specific_call()
#endif
"""

This makes the code difficult to review/understand/test/maintain. The
function was even checked without being called at some point (removed
as of r35969). I regard this as a proof that such a pattern is very
error-prone.

Can you consider moving these blocks to engine-specific functions?
Maybe they would fit somewhere in the existing functions, or maybe the
interface has to evolve. If so, find the appropriate abstractions, add
corresponding calls to the engine operations and implement them as
such (they can be left empty for the other engines if irrelevant, no
problem with that of course)...

I would have nacked the change if asked. Now I'm fine with not
reverting r36093 if you can address the issues above. Otherwise I
consider that it introduces a significant technical debt, despite how
nice the feature is! It's good work, but not ready to land IMHO.

Tudor, Fyodor, Dan? What do you think?

--
Henri

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: Nsock IOCP engine review

Henri Doreau
Hello,

can you start with these changes?
  - move IOCP-specific structures from struct npool to engine_data;
  - remove useless strcmp(nsp->engine->name, "iocp") from nsock_iocp.c calls;
  - refactor nsock_iocp.c code a bit, there is a lot of duplicated code there;
  - replace assertions by proper error handling;
  - merge nsock_iocp.c to engine_iocp.c.


Now for the engine API change:

2016-08-10 23:30 GMT+02:00 Tudor-Emil COMAN <[hidden email]>:
> To eliminate that anti-pattern we would need to add the following functions
> to io_engine.
>
>
> int(*event_initiate)(struct npool *nsp, struct nevent *nse);
> int(*event_terminate)(struct npool *nsp, struct nevent *nse);

Can this fit into nsock_engine_iod_{register,modify,unregister}? By
passing in the struct nevent for example?

> int(*iod_connect)(struct npool *nsp, int sockfd, const struct sockaddr
> *addr, socklen_t addrlen);
> int(*iod_read)(struct npool *nsp, int sockfd, void *buf, size_t len, int
> flags,
>                       struct sockaddr *src_addr, socklen_t *addrlen);
> int(*iod_write)(struct npool *nsp, int sockfd, const void *buf, size_t len,
> int flags,
>                        const struct sockaddr *dest_addr, socklen_t addrlen);
>
This sounds reasonable, according to what I can read about IOCP. Let's
go for a struct io_operations {...}, referenced by the engine
definitions. Two statically defined ones: posix & iocp.

> For any other engine except iocp:
>   - event_initiate and event_terminate are empty
>   - iod_connect, iod_read, iod_write are wrappers over connect, sendto,
> recvfrom
>
> I took all the code from nsock_iocp.c and added it to engine_iocp.c.
> Everything I added to struct npool can be moved to iocp_engine_info.
>
> With these we can contain everything iocp related to engine_iocp.c.
>
> Are these changes acceptable?
>
> Cheers,
> Tudor
>

Please apply the changes to a separate branch (or send patches to the
list), so that they can be reviewed first. Once ready and before
merging into svn trunk, do not hesitate to call for testing on the
list. Many people have specific setups, scanning practices etc. It is
an excellent way to actually stress things and uncover defects before
they even touch svn trunk.

Regarding testing: have you script/version-scanned SSL-enabled
services with the IOCP engine? Are the results perfectly identical
regardless the selected engine? What about proxy support? What about
pcap?

Regards

--
Henri
_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/
Reply | Threaded
Open this post in threaded view
|

Re: Nsock IOCP engine review

Tudor-Emil COMAN

Hi,


I'll push the changes to the branch in nmap-exp: https://svn.nmap.org/nmap-exp/tudor/nsock-iocp/


We can fit event_initiate and event_terminate in iod_register/iod_unregister.


Both script scanning(I tried ssl-cert) and version scanning work as the other engines do.

Proxy and pcap support I haven't tested, but I'll come back with more info on this.



Regards,

Tudor




From: Henri Doreau <[hidden email]>
Sent: Thursday, August 11, 2016 4:07:31 PM
To: Tudor-Emil COMAN
Cc: Nmap dev
Subject: Re: Nsock IOCP engine review
 
Hello,

can you start with these changes?
  - move IOCP-specific structures from struct npool to engine_data;
  - remove useless strcmp(nsp->engine->name, "iocp") from nsock_iocp.c calls;
  - refactor nsock_iocp.c code a bit, there is a lot of duplicated code there;
  - replace assertions by proper error handling;
  - merge nsock_iocp.c to engine_iocp.c.


Now for the engine API change:

2016-08-10 23:30 GMT+02:00 Tudor-Emil COMAN <[hidden email]>:
> To eliminate that anti-pattern we would need to add the following functions
> to io_engine.
>
>
> int(*event_initiate)(struct npool *nsp, struct nevent *nse);
> int(*event_terminate)(struct npool *nsp, struct nevent *nse);

Can this fit into nsock_engine_iod_{register,modify,unregister}? By
passing in the struct nevent for example?

> int(*iod_connect)(struct npool *nsp, int sockfd, const struct sockaddr
> *addr, socklen_t addrlen);
> int(*iod_read)(struct npool *nsp, int sockfd, void *buf, size_t len, int
> flags,
>                       struct sockaddr *src_addr, socklen_t *addrlen);
> int(*iod_write)(struct npool *nsp, int sockfd, const void *buf, size_t len,
> int flags,
>                        const struct sockaddr *dest_addr, socklen_t addrlen);
>
This sounds reasonable, according to what I can read about IOCP. Let's
go for a struct io_operations {...}, referenced by the engine
definitions. Two statically defined ones: posix & iocp.

> For any other engine except iocp:
>   - event_initiate and event_terminate are empty
>   - iod_connect, iod_read, iod_write are wrappers over connect, sendto,
> recvfrom
>
> I took all the code from nsock_iocp.c and added it to engine_iocp.c.
> Everything I added to struct npool can be moved to iocp_engine_info.
>
> With these we can contain everything iocp related to engine_iocp.c.
>
> Are these changes acceptable?
>
> Cheers,
> Tudor
>

Please apply the changes to a separate branch (or send patches to the
list), so that they can be reviewed first. Once ready and before
merging into svn trunk, do not hesitate to call for testing on the
list. Many people have specific setups, scanning practices etc. It is
an excellent way to actually stress things and uncover defects before
they even touch svn trunk.

Regarding testing: have you script/version-scanned SSL-enabled
services with the IOCP engine? Are the results perfectly identical
regardless the selected engine? What about proxy support? What about
pcap?

Regards

--
Henri

_______________________________________________
Sent through the dev mailing list
https://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/