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

TCP port in ZMQ_LAST_ENDPOINT depends on global locale #3385

Closed
carrotIndustries opened this issue Feb 4, 2019 · 6 comments
Closed

TCP port in ZMQ_LAST_ENDPOINT depends on global locale #3385

carrotIndustries opened this issue Feb 4, 2019 · 6 comments

Comments

@carrotIndustries
Copy link

Issue description

The TCP port formatting in ZMQ_LAST_ENDPOINT depends on the locale set by std::locale::global. With thousands grouping enabled, the port number contains commas. UDP and other ports are likely affected as well.

Environment

  • libzmq version 4.3.1
  • OS: Arch Linux

Minimal test code / Steps to reproduce the issue

#include <zmq.h>
#include <assert.h>
#include <iostream>
#include <sstream>

int main(void) {
    struct comma : std::numpunct<char> {
        char do_thousands_sep() const override
        {
            return ',';
        } // comma
        std::string do_grouping() const { return "\3"; } // groups of 3 digits
    };
    std::locale::global(std::locale(std::locale("C"), new comma));
    void *context = zmq_ctx_new ();
    void *sock = zmq_socket (context, ZMQ_REP);
    int rc = zmq_bind (sock, "tcp://127.0.0.1:*");
    assert(rc == 0);
    char ep[1024];
    size_t sz = sizeof(ep);
    zmq_getsockopt(sock, ZMQ_LAST_ENDPOINT, ep, &sz);
    std::cout << "EP " << ep << std::endl;
}

What's the actual result?

EP tcp://127.0.0.1:39,375

What's the expected result?

EP tcp://127.0.0.1:39375

Possible fix

In https://github.com/zeromq/libzmq/blob/master/src/tcp_address.cpp#L134, use s.imbue(std::locale("C"));

@bluca
Copy link
Member

bluca commented Feb 4, 2019

In https://github.com/zeromq/libzmq/blob/master/src/tcp_address.cpp#L134, use s.imbue(std::locale("C"));

Sounds about right, could you please send a PR?

@carrotIndustries
Copy link
Author

Sounds about right, could you please send a PR?

As far as I can tell MSVC doesn't support the "C" locale: https://stackoverflow.com/a/4497266 I'd rather not have ugly #ifdefs in that code. Any ideas on resetting the locale to something sensible in a portable way?

@bluca
Copy link
Member

bluca commented Feb 4, 2019

I'm afraid it's outside of my area - @sigiesec any idea?

@bluca
Copy link
Member

bluca commented Feb 4, 2019

but if there isn't any, better an ugly ifdef than an ugly bug

@sigiesec
Copy link
Member

sigiesec commented Feb 4, 2019

I think it would be preferable to change this to use sprintf, e.g., so that this isn't dependent on the locale at all.

@carrotIndustries
Copy link
Author

If I read the docs correctly, std::locale::classic() should have the same effect as std::locale("C") while being portable.

sigiesec added a commit to sigiesec/libzmq that referenced this issue Feb 5, 2019
Solution: use sprintf instead of std::stringstream
Fixes zeromq#3385
sigiesec added a commit to sigiesec/libzmq that referenced this issue Feb 5, 2019
Solution: use sprintf instead of std::stringstream
Fixes zeromq#3385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants