Libzmq: Problem: curve_server steps on stdout.

[evoskuil]

There are about 20 statements in curve_server_t that write directly to stdout, such as:

            //  Temporary support for security debugging
            puts ("CURVE I: invalid handshake command");

It's marked as temporary. Is there a plan afoot for an implementation without this side effect?

[bluca]

Yes we should take these away, especially now that there are socket monitor events.

@vtellier do you think any of those "puts" calls in curve_server.cpp could instead use event monitoring? Perhaps define error numbers and return them as the value of the event

[vtellier]

Yes, this really makes sense.

Actually I though about that while working on #2227 but I didn't implemented it because curve_server is used by stream_engine through the interface mechanism_t and the events are signaled by stream_engine, regardless the kind of mechanism currently in use.
This makes sense: the first level of information requested in #2227 (success / failure) is defined by stream_engine.

A solution for this issue is to extend a bit mechanism_t so its implementations have the possibility to give more details, and then get rid of these puts.

[vtellier]

@bluca Here is one approach to solve this, see the comment of 7c3093dbe7dbf5babfcabad2a725bd9b6b658d95 for details.
The implementation is not completed, this is just a draft.

[bluca]

Sounds like a good starting point!
Maybe "undefined" rather than "no_detail"

[vtellier]

FYI I don't know if I'll have time to continue on this issue, if anyone wants to go ahead just feel free.

[evoskuil]

I got burned by this issue again. I shipped the 4.2.20 nuget package with these console writes enabled. That's actually a big deal, so I'll need to redeploy. I'm not familiar with the monitoring code, but I would really like to get these console writes out of the lib.

[sigiesec]

I started work on this again in my branch https://github.com/sigiesec/libzmq/tree/curve-events

[bluca]

Great, thanks!

There was already part of the work done here: https://github.com/zeromq/libzmq/pull/2473

[sigiesec]

@bluca, I am aware of that :) I started where @vtellier left.

[sigiesec]

This morning, I achieved an intermediate state where both Unix (https://travis-ci.org/sigiesec/libzmq/builds/260100852) and Windows (https://ci.appveyor.com/project/sigiesec/libzmq/build/build-14) builds worked. This still contained a workaround regarding a spurious ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL event, which I am currently investigating.

However, I am wondering whether the event classification is good in the current status:

ZMQ_EVENT_HANDSHAKE_FAILED_PROTOCOL is used for both protocol errors with the ZMTP/CURVE peer and with the ZAP handler. I would split these up into two types (maybe ZMQ_EVENT_HANDSHAKE_FAILED_ZMTP , ZMQ_EVENT_HANDSHAKE_FAILED_ZAP) as the two conversations are independent from each other, and errors have a completely different origin.

What do you think about that?

[bluca]

Sure, that sounds like a good plan

[sigiesec]

Okay, I did the split-up of the event types.

There are still issues with a spurious extra event of type ZMQ_EVENT_HANDSHAKE_FAILED_NO_DETAIL with an error code of EPIPE (i.e. the peer closed the connection). Now, these only seem to happen under Windows, and not reproducibly. Sometimes I see them while debugging locally, but I have not yet been able to break where the client closes the connection to analyze why. Any ideas why this might happen?

[bluca]

Really not sure, sorry - is that on appveyor? Could it be overloaded VMs?

Also just a nit since I guess you'll be rebasing/squashing soon - there's a few white-space changes in the diff, notably removing the space between a function/statement and the opening bracket. Note that the style guideline we follow does require that space, eg: if (foo) rather than if(foo) :-)

[sigiesec]

Yes it's on appveyor, e.g. in https://ci.appveyor.com/project/sigiesec/libzmq/build/build-21, 3 builds are failing due to such an event, the others are fine. I think it is timing-related, but not sure if this is a behavior that must be expected (and then if it makes sense to emit such an event under these circumstances), or if there is some underlying problem.

Regarding formatting: Ok... I will check my changes for that. However, I noticed that the rest of the code doesn't do this consistently either. Is there a clang-format config or sth. that could be used to format the whole codebase consistently?

[bluca]

There's no config to automate it, we occasionally sweep the repo and try to catch them beforehand if possible. Sorry to be pedantic :-)

[bluca]

Regarding Windows, given there's no functional change in the protocol in the diff, as far as I can see, I think it is reasonable to assume that's how it works at the moment and expect that event so that the test doesn't fail.
Eventually file another ticket to keep track of it, but if you can't figure out exactly what's going on I would recommend not treating it as a blocker.

[sigiesec]

Ok, I will try to assume the current behavior in the test assertions. I also think the changes do not affect the protocol between the peers at all.

I reformatted the code. It is quite hard without a tool, especially given that the rules stated under http://zeromq.org/docs:style are not comprehensive, and there are many violations in the current code base. I made up a clang-format config file for myself, but I think there are some rules that clang-format does not currently support. But maybe this should be discussed outside of this ticket :)

[bluca]

Great, thanks!

[bluca]

Fixes by #2645