Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[issues/217] mlm_client_set_consumer broke the stream when pattern is * #22

Merged
merged 4 commits into from
Mar 8, 2019

Conversation

geraldguillaume
Copy link

@geraldguillaume geraldguillaume commented Mar 5, 2019

Problem : zeromq#217
if pattern "*" is used, the related stream will be broken and mlm broker will start overcosumming 100% CPU forever

solution : if pattern is * mlm_set_consumer reject it

Note : as mlm_client_engine.inc is auto generated by gsl (https://github.com/zeromq/zproto/blob/master/src/zproto_client_c.gsl), it's quite really complex to handle this issue in a clean way. This is why we took a different approach, so this patch is not pushed on upstream.

Signed-off-by: Gerald Guillaume [email protected]

@boricj
Copy link

boricj commented Mar 5, 2019

I would fail the call to mlm_client_set_consumer() instead. * is not a valid regex pattern, we should not hide a latent bug that'll explode whenever upstream will be properly fixed.

@geraldguillaume
Copy link
Author

@boricj, the issue is that 'zrex_valid()' is wrong and returns true when checking "*" (as it was mentionned in zeromq#217).
So we can't trust zrex_valid() to validate the reggex.
As this issue is not yet fixed in upstream, I did this workaround.

@geraldguillaume
Copy link
Author

would you prefer something like if (streq(pattern,"*")) return -1;

@boricj
Copy link

boricj commented Mar 8, 2019

Yes. We should reject invalid regexes, not silently fix them up.

@boricj boricj merged commit 90b2995 into 42ity:1.0-FTY-master Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants