Address comments from PR 812: Add check before deserializing messages

[zbud-msft]

Comment 1

@zbud-msft , I don't think the envelopes message is the root cause of bug #12570, because according to the document you past:

https://zguide.zeromq.org/docs/chapter3/
When you use REQ and REP sockets you don’t even see envelopes; these sockets deal with them automatically.

Do you have log or test case to show how ZMQ receive the "envelope delimiter" and pass it to deserializer?

The new code here is just ignore those short message, but as my understand the "envelope delimiter" should not never deserialize, you need add code in the socket receive code to ignore the delimiter there.

Response

After reviewing, I also realize that the envelope delimiter is not the RC for this issue. After reviewing the code, we realize that the message is a control character being read by zmg_msg_recv, which when displayed as c_str() is '001' and of length 1. I have opened an issue with libzmq about this issue, but will drop messages of length 1 and less for now zeromq/libzmq#4587:

Comment 2

This new code in UT doesn't relate with ZMQ. the source2 not send/receive via ZMQ, why put it here?

I suggest you create a new UT:

1. send message via ZMQ
2. receiver socket add code to check and ignore "envelope delimiter"
3. In UT check the signal (the signal can be a log message) that the "envelope delimiter" been ignored and not cause error.

Response

Since we are not dealing with "envelope delimiter", I have created a UT where zmq socket sends messages with empty source and empty data

[qiluo-msft]

@liuh-80 Could you help review?

[qiluo-msft]

Please check with other reviewers.

[liuh-80]

The Ut can pass without any code change in zmq_read_part, which means the UT change is not necessary.

[liuh-80]

By read the eventd code, I found it's using event_service.
So you need create new UT to cover channel_read in https://github.com/sonic-net/sonic-swss-common/blob/master/tests/events_service_ut.cpp

[liuh-80]

Discussion offline ,suggest revert this PR.

[liuh-80]

I check eventd code, it's not use zmq_read_part, how this change can fix the original eventd issue.

[liuh-80]

By read eventd code, I found it's a code bug in channel read, please check following code and fix it:

int
event_service::channel_read(int &code, event_serialized_lst_t &data)
{
event_serialized_lst_t().swap(data);
return zmq_message_read(m_socket, 0, code, data); <== code is a int*
}

template<typename P1, typename P2>
int
zmq_message_read(void *sock, int flag, P1 &pt1, P2 &pt2)
{
auto render = boost::serialization::singleton::get_const_instance();

return render.zmq_message_read(sock, flag, pt1, pt2); <-- pt1 is a int*

}

template<typename P1, typename P2>
int
zmq_message_read(void *sock, int flag, P1 &pt1, P2 &pt2)
{
    int more = 0, rc, rc2 = 0;

    rc = zmq_read_part(sock, flag, more, pt1); <== pt1 is a int*

    if (more) {
        /*
         * read second part if more is set, irrespective
         * of any failure. More is set, only if sock is valid.
         */
        rc2 = zmq_read_part(sock, 0, more, pt2);
    }




template<typename DT>
int 
zmq_read_part(void *sock, int flag, int &more, DT &data) <== data maybe a int*
{
    zmq_msg_t msg;

    more = 0;
    zmq_msg_init(&msg);
    int rc = zmq_msg_recv(&msg, sock, flag);
    if (rc != -1) {
        size_t more_size = sizeof (more);

        zmq_getsockopt (sock, ZMQ_RCVMORE, &more, &more_size);

        rc = zmsg_to_map(msg, data); <== data here maybe a int* and msg maybe a int
        RET_ON_ERR(rc == 0, "Failed to deserialize part rc=%d", rc);
        /* read more flag if message read fails to de-serialize */
    }



template <typename Map>
int
zmsg_to_map(zmq_msg_t &msg, Map& data)
{
    string s((const char *)zmq_msg_data(&msg), zmq_msg_size(&msg));
    return deserialize(s, data); <== may try to deserialize a int data to a int pointer
}

[liuh-80]

May not related with original issue, suggest create UT in other PR to fix.
#closed

[qiluo-msft]

Offline discussed, this PR does not reach the root issue and does not help to solve the issue and only suppress error messages. Will explore other solutions.