Issue37228
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2019-06-11 09:36 by vaizki, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 17311 | merged | aeros, 2019-11-21 03:00 | |
PR 17529 | merged | miss-islington, 2019-12-09 14:21 | |
PR 17570 | merged | ned.deily, 2019-12-11 04:29 | |
PR 17571 | merged | aeros, 2019-12-11 06:21 | |
PR 17577 | merged | aeros, 2019-12-12 07:35 | |
PR 17579 | merged | miss-islington, 2019-12-12 13:49 | |
PR 17580 | merged | miss-islington, 2019-12-12 13:49 | |
PR 17581 | merged | miss-islington, 2019-12-12 13:49 | |
PR 17595 | merged | aeros, 2019-12-14 02:02 | |
PR 17630 | merged | aeros, 2019-12-16 22:49 | |
PR 17631 | merged | aeros, 2019-12-16 22:50 | |
PR 17632 | merged | aeros, 2019-12-16 22:50 |
Messages (64) | |||
---|---|---|---|
msg345209 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-06-11 09:36 | |
When using loop.create_datagram_endpoint(), the default for reuse_address=True, which sets the SO_REUSEADDR sockopt for the UDP socket. This is a dangerous and unreasonable default for UDP, because in Linux it allows multiple processes to create listening sockets for the same UDP port and the kernel will randomly give incoming packets to these processes. I discovered this by accidentally starting two Python asyncio programs with the same UDP port and instead of getting an exception that the address is already in use, everything looked to be working except half my packets went to the wrong process. The documentation also refers to behavior with TCP sockets: https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.create_datagram_endpoint "reuse_address tells the kernel to reuse a local socket in TIME_WAIT state, without waiting for its natural timeout to expire. If not specified will automatically be set to True on Unix." This is true for TCP but not UDP. Either the documentation should reflect the current (dangerous) behavior or the behavior should be changed for UDP sockets. Workaround is of course to create your own socket without SO_REUSEADDR and pass it to create_datagram_endpoint(). |
|||
msg345210 - (view) | Author: Andrew Svetlov (asvetlov) * | Date: 2019-06-11 09:39 | |
There is no chance to change the flag for 3.7. But we can consider it for 3.8 Yuri, what do you think? |
|||
msg356854 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-18 10:00 | |
Can we still consider this for 3.9? I still think this is an easy way to accidentally blow your foot off with something that will probably only show up in production. |
|||
msg356855 - (view) | Author: Andrew Svetlov (asvetlov) * | Date: 2019-11-18 10:18 | |
I see two proposals: 1. Change the default for reuse_address flag 2. Document existing behavior as dangerous. What is the suggestion? Pick one, please :) |
|||
msg356856 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-18 10:48 | |
I definitely propose changing the default for UDP sockets. Having multiple processes binding to a the same port and load-balancing incoming UDP traffic intentionally is a rare scenario which should be supported but not the default. For TCP the default is reasonable because everyone creating server sockets will usually want to set it. |
|||
msg356876 - (view) | Author: Yury Selivanov (yselivanov) * | Date: 2019-11-18 16:17 | |
I'd be -1 on changing the default of an existing method, at least without consulting with a wider audience. We can add a new method to the loop and deprecate create_datagram_endpoint. I suggest to post to python-dev and discuss this before making any decisions. |
|||
msg356897 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-18 18:51 | |
Sure, I fully appreciate implications of changing default behaviour and will post on python-dev. |
|||
msg356909 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2019-11-18 20:59 | |
I agree that this is a bad default (and whoever wrote it, probably me, didn't know what this does for UDP). I think the right solution is to change the default, not to introduce a new method. Maybe we can deprecate the default somehow? There currently is a line the computes the default based on platform: if reuse_address is None: reuse_address = os.name == 'posix' and sys.platform != 'cygwin' We could change this to issue a deprecation warning stating that in the future the default will change to False everywhere. Then in the next release we could change the default to False and remove the warning. |
|||
msg356910 - (view) | Author: Nathaniel Smith (njs) * | Date: 2019-11-18 21:15 | |
Ouch, that's nasty. It also has security implications. For example, suppose you have a multi-user computer, where one user is has a video call going, which transfers packets over UDP. Another user could check what port they're using, bind to the same port, and steal half the packets, letting them effectively listen in on the first user's phone call. Well, hopefully most conferencing tools use end-to-end encryption. But the point is that we don't normally let unprivileged users steal network traffic intended for other users. |
|||
msg356927 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-18 22:59 | |
How much harm would there be in bringing the DeprecationWarning into the next patch of existing (3.6, 3.7, 3.8) releases? The security implications are significant enough that I'd want to be notified of it in my software ASAP. Users can (and should!) squelch the warning by passing the setting explicitly. |
|||
msg356956 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-19 09:07 | |
I had a quick search through github for calls to create_datagram_endpoint() and the reuse_address is either not set or set explicitly to True, probably due to the error in the documentation. Only in one case (of my admittedly small sample) did it seem like the parameter was set to True intentionally to get the behavior of binding multiple processes to the same port. Due to the nature of the function, it is also often buried inside packages implementing higher level protocols (SIP, STUN, DNS, RTP) which want to create a convenience function to create asyncio servers and clients. The deprecation warning and getting it into 3.6 and later sounds very reasonable to me. |
|||
msg357011 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-20 00:33 | |
I'm working on patches for the deprecation bits (targeting 3.6 for now; will work my way up from there) for review, including documentation. Unless someone tells me to stop. :-) In an attempt to make this not-so-Linux-specific, I'm reviewing how this works on non-Linux platforms (MacOS, FreeBSD) as well. Reading Linux's socket(7) man page makes it seem like reuse_port is the issue (but that actually has protections to ensure you're only doing it across the same UID at least). I had to write my own test jig to (re-)convince myself that, indeed, reuse_addr is the problem. |
|||
msg357018 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-20 01:33 | |
FreeBSD 12.1 and MacOS 10.15.1 (Catalina) appear to have saner and safer behavior. Both require the use of SO_REUSEPORT for this behavior to happen as well. FreeBSD also requires the UID to be the same or 0 for subsequent processes to make the bind() call. I'll call this out as being Linux-specific in the deprecation message for now. (I don't have an AIX, HP-UX, or Solaris system handy to test, nor do I really want one if I'm being honest. :-) .) |
|||
msg357039 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-20 08:26 | |
Alright -- my first stab at the DeprecationWarning in 3.6. https://github.com/dacut/cpython/commit/6a1e261678975e2c70ec6b5e98e8affa28702312 Please critique away, and don't fret about bruising my ego. :-) Is there a more idiomatic way of getting a warning to show up against the first callee that's not in the current module? I'm not enamored of the monstrosity I'm put together around line 918 of base_events.py (but less enamored about displaying warnings that the user is going to tear their hair out over trying to find the offending source line): https://github.com/dacut/cpython/commit/6a1e261678975e2c70ec6b5e98e8affa28702312#diff-08afa52ab2b1511bee8527814ad44d80R918 |
|||
msg357040 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-20 08:59 | |
David, in terms of documentation changes and the emitted deprecation warning itself, I think it would be appropriate to instruct that please set the parameter explicitly to True or False to silence the warning AND point out that setting it to True has significant security and previously incorrectly documented functional implications. Now your updated docs and warning read more like we are working around a Linux security bug which is not really the case - this behavior was intentionally added to the kernels and some of the code I do for a living relies on it to work properly. Admittedly the restriction of having the same UID wouldn't hurt. And browsing again through the hits to my github searches, it makes me cringe how many people are already explicitly setting reuse_address=True in their code because the current documentation mistakenly makes it seem harmless and desirable. Makes me wonder if we need to put out a CVE? At the very least, I will be putting in PRs to the asyncio packages that I myself use and understand. |
|||
msg357042 - (view) | Author: Nathaniel Smith (njs) * | Date: 2019-11-20 09:21 | |
> Now your updated docs and warning read more like we are working around a Linux security bug which is not really the case - this behavior was intentionally added to the kernels and some of the code I do for a living relies on it to work properly. Admittedly the restriction of having the same UID wouldn't hurt. I think you can use SO_REUSEPORT instead, and for UDP sockets it's identical to SO_REUSEADDR except with the same-UID restriction added? If that's right then it might make sense to unconditionally switch SO_REUSEADDR -> SO_REUSEPORT, even in existing Python releases – on the theory that it fixes the main security hole, while being back-compatible enough to be acceptable for a point release. |
|||
msg357049 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-20 10:17 | |
> I think you can use SO_REUSEPORT instead, and for UDP sockets it's identical to SO_REUSEADDR except with the same-UID restriction added? > If that's right then it might make sense to unconditionally switch SO_REUSEADDR -> SO_REUSEPORT, even in existing Python releases – on the theory that it fixes the main security hole, while being back-compatible enough to be acceptable for a point release. +1, I think this is the best proposal so far. From my reading of http://man7.org/linux/man-pages/man7/socket.7.html (specifically the SO_REUSEPORT section) this seems correct. Emitting a `DeprecationWarning` and a delayed change for the default value of `reuse_address` not only leaves open a security concern for the near future (until the change is actually made); it also has a maintenance cost for users and libraries. Also, changing the default value wouldn't address the problem for those who explicitly specified `reuse_address=True`. The way I see it, changing `socket.SO_REUSEADDR` to `socket.SO_REUSEPORT` would apply the security fix more rapidly, fix the issue for more users, and have a much lower (if not zero) maintenance cost for the majority of users. This would also allow us to apply the change to more previous Python versions, since it doesn't affect asyncio's public API. If we're just changing `socket.SO_REUSEADDR` to `socket.SO_REUSEPORT` at: https://github.com/python/cpython/blob/7eee5beaf87be898a679278c480e8dd0df76d351/Lib/asyncio/base_events.py#L1321 We should be able to apply to change starting from Python 3.5+, no? I should have the time to open a PR with backports within the next day if needed, particularly since it's a fairly small code change. I'll leave that up to Nathaniel though since he came up with the idea, let me know either way. |
|||
msg357066 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-20 12:18 | |
Going to SO_REUSEPORT will fix the security issue and emitting a deprecation warning for default value invocation will catch the eyes of some maintainers but it will not prevent what caused me to catch this issue in the first place - starting two processes (with same UID) accidentally listening on the same UDP port (in my case it was port 5060 as a default SIP port). Since there already is a reuse_port parameter to create_datagram_endpoint(), I assume the proposal is to set default value for reuse_addresss=False and reuse_port=True? But if reuse_address is explicitly set to True, are we going to just set SO_REUSEPORT instead and always leave SO_REUSEADDR unset? This would leave the reuse_address parameter completely useless and still allow accidental port reuse. What if I really do want SO_REUSEADDR? Ok I can create a socket separately, call setsockopt() on it and pass it as the sock parameter to create_datagram_endpoint(). Maybe I'm not fully grasping the proposal.. or maybe we should just deprecate reuse_port from both create_datagram_endpoint() and create_server() + reuse_addr from create_datagram_endpoint()? This would leave the TCP create_server() with the reuse_addr parameter, defaulting reasonably to True. To use TCP/UDP SO_REUSEPORT or UDP SO_REUSEADDR, the docs would tell you to bake your own socket with socket.socket(). Those few (like me) who really need the functionality can survive without all-in-one convenience functions on asyncio.loop |
|||
msg357068 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2019-11-20 12:25 | |
(previous message deleted, I hadn't noticed the "reuse_port" parameter) My preference for create_datagram_endpoint() would be: - make the "reuse_address" parameter a no-op, and raise an error when "reuse_address=True" is passed - do that in 3.8 as well This way we 1) solve the security issue 2) ensure that anyone that passed "reuse_address=True" explicitly is notified that they were doing something dangerous unwillingly. And, yes, someone who really wants SO_REUSEADDR can set it manually, for example by calling `transport.get_extra_info('socket')`. |
|||
msg357101 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-20 20:27 | |
Jukka -- Fair enough; will reword this a bit. I'm trying to keep the DeprecationWarning short enough so people's eyes don't glaze over; will see what wordsmithing I can do here. (Once you get beyond a certain length, the number of folks who actually read the message goes down the longer it is.) |
|||
msg357103 - (view) | Author: David Cuthbert (dacut) * | Date: 2019-11-20 21:04 | |
On the completely deprecate reuse_address and rewrite/force folks to use reuse_port proposals, I'm a bit dubious of this approach. Right now, we have two knobs that directly correspond to (potential) kernel-level socket parameters, SO_REUSEADDR and SO_REUSEPORT. We just chose an unfortunate default for one of them. Otherwise, the behavior is nicely explicit. Rewriting or forbidding it, even when the user has (essentially) acknowledged they're accepting risks by passing reuse_address=True, seems to fly in the face of this. Users get bitten when we start convoluting logic here (I was just bitten by fcntl.lockf() not actually calling lockf() last month and ended up having to call lockf() via ctypes.). There are some platforms (Linux pre-3.9 kernels) that don't have SO_REUSEPORT. I wish I could say I don't care about such platforms; alas, I just had to compile Python 3.7 on a system running a 2.6 kernel last month at a client site. Finally, I've only scratched the surface with my test cases, on single-homed systems, IPv4, and binding to 0.0.0.0. There are a lot of parameters to try here before we decree that reuse_address has no purpose existing, and an OS could later reimplement saner defaults on SO_REUSEADDR. Changing a default already has some controversy (Yury was -1 on this change). This seems much bigger and I'm not convinced we fully understand its ramifications. That said, I'll happily implement whatever the consensus ends up being, or consensus as designated by the vBDFL, or wherever that decision needs to come from. :-) |
|||
msg357108 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-20 22:35 | |
> There are some platforms (Linux pre-3.9 kernels) that don't have SO_REUSEPORT. I wish I could say I don't care about such platforms; alas, I just had to compile Python 3.7 on a system running a 2.6 kernel last month at a client site. Based on https://www.kernel.org/releases.html, pre-3.9 Linux kernels are no longer supported upstream, the oldest LTS version is 3.16. There are likely a number of systems running older unsupported kernel versions, but I don't think that we can be reasonably expected to maintain indefinite backwards compatibility to versions that aren't supported upstream, especially not at the expense of the ones that are supported. In those cases, I think the responsibility ultimately falls upon the owners of the system or third party group providing service. This particular fix would be fairly straightforward: > And, yes, someone who really wants SO_REUSEADDR can set it manually, for example by calling `transport.get_extra_info('socket')`. |
|||
msg357109 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-20 22:49 | |
> We just chose an unfortunate default for one of them I'd like to point out that it is also documented completely wrong up to this point in time and thus people who chose True are most likely to be unaware of the actual consequences. A user's explicit choice based on misinformation is not really demonstrating intent to shoot oneself in the foot. So after changing my mind a couple of times I'm still attached to the idea that in 3.10 reuse_address and reuse_port are both removed and the tiny portion of developers who need them will just create a suitably setsockopt'd socket and pass it to the function. 2 lines of additional code is all it takes. If the documentation had been correct all along and just the default bad, this mess would be much smaller. |
|||
msg357115 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-11-21 00:26 | |
(Provisionally marking this as a security-related deferred blocker issue for backporting) |
|||
msg357123 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 02:19 | |
> I'd like to point out that it is also documented completely wrong up to this point in time and thus people who chose True are most likely to be unaware of the actual consequences. A user's explicit choice based on misinformation is not really demonstrating intent to shoot oneself in the foot. That's why I'm in favor of making the internal change from `SO_REUSEADDR` to `SO_REUSEPORT` (as Nathaniel suggested) and raising a warning for an explicit `reuse_address=True` (as Antoine suggested). If someone has a genuine need for using SO_REUSEADDR, they can do so manually. This will patch the issue without any maintenance cost to users that didn't explicitly pass `reuse_address=True`, and raise a warning if they did pass it. The majority of users seem to have done did the former, which would make sense. Compare the following GitHub code search results: All usage of `loop.create_datagram_endpoint`: https://github.com/search?q=%22loop.create_datagram_endpoint%22&type=Code (~3,500 Python results) Usage of `loop.create_datagram_endpoint` + `reuse_address=True`: https://github.com/search?q=%22loop.create_datagram_endpoint%22+%22reuse_address%3DTrue%22&type=Code (~650 Python results) Note: the second set of results contains some false positives due to usage of `reuse_address=True` for something else in the same code file. So, the actual number is likely less. > I'm still attached to the idea that in 3.10 reuse_address and reuse_port are both removed IIUC, there's only a security concern with the usage of `SO_REUSEADDR`, not `SO_REUSEPORT`. So there's no need at all to remove or deprecate the `reuse_port` parameter. > (Provisionally marking this as a security-related deferred blocker issue for backporting) Thanks, Ned. |
|||
msg357124 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 02:33 | |
> My preference for create_datagram_endpoint() would be: > - make the "reuse_address" parameter a no-op, and raise an error when "reuse_address=True" is passed > - do that in 3.8 as well This solution would more elegant, but my concern is that it will leave open a substantial security concern for versions 3.5, 3.6, and 3.7 if we only applied that to 3.8 and 3.9. Although Nathaniel's suggestion would ultimately make the `reuse_address` parameter fairly meaningless (essentially converting it into an alias for `reuse_port`), it would fix the security issue while still maintaining backwards compatibility for most users, allowing us to backport it all the way back to 3.5. Perhaps this could lead to a deprecation warning or error, and then an eventual removal of `reuse_address`, but I think that's more of a long term concern for 3.9+. |
|||
msg357126 - (view) | Author: Yury Selivanov (yselivanov) * | Date: 2019-11-21 02:39 | |
> My preference for create_datagram_endpoint() would be: > - make the "reuse_address" parameter a no-op, and raise an error when "reuse_address=True" is passed > - do that in 3.8 as well Yeah, I like this prposal; we can apply this to all Python's from 3.5 to 3.8. With a proper documentation update this should be OK. |
|||
msg357127 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 03:15 | |
> Yeah, I like this prposal; we can apply this to all Python's from 3.5 to 3.8. With a proper documentation update this should be OK. Oh in that case, would you like me to close or modify GH-17311? I didn't think you'd approve of making the more extensive changes all the way back to 3.5. |
|||
msg357128 - (view) | Author: Yury Selivanov (yselivanov) * | Date: 2019-11-21 03:18 | |
> Oh in that case, would you like me to close or modify GH-17311? I didn't think you'd approve of making the more extensive changes all the way back to 3.5. After reading the comments here I think Antoine's solution makes sense. But... let's wait a bit to see what other people say. My comment is just a single +1; maybe Andrew, Nathaniel, or Guido have arguments against it. |
|||
msg357131 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2019-11-21 05:50 | |
(Don't wait for me, I am preoccupied with other things this week.) |
|||
msg357143 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 08:47 | |
So after trying a few different implementations, I don't think the proposal to simply change `SO_REUSEADDR` -> `SO_REUSEPORT` will work, due to Windows incompatibility (based on the results from Azure Pipelines). `SO_REUSEADDR` is supported on Windows, but not `SO_REUSEPORT`. So, if we essentially make the `reuse_address` parameter an alias for `reuse_port`, it will lead to some rather confusing errors for Windows users that are explicitly passing `reuse_address=True` for `create_datagram_endpoint()`. I don't think having incompatibility with Windows users is a viable option. As a result, I will make the modifications to GH-17311 to implement Antoine's proposal. I'm not a huge fan of the user maintenance cost that it will cause for those who are currently passing `reuse_address=True` in 3.5+. But IMO, that's a lot better than leaving open a significant security vulnerability for older supported versions of Python. |
|||
msg357145 - (view) | Author: Nathaniel Smith (njs) * | Date: 2019-11-21 09:00 | |
I was assuming we'd only do this on Linux, since that's where the bug is... though now that you mention it the Windows behavior is probably wonky too. SO_REUSEADDR and SO_REUSEPORT have different semantics on all three of Windows/BSD/Linux. A higher-level interface like asyncio doesn't necessarily want to map its kwargs directly to non-portable low-level options like this. |
|||
msg357154 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 10:26 | |
> I was assuming we'd only do this on Linux, since that's where the bug is... though now that you mention it the Windows behavior is probably wonky too. Yeah, but I'm not confident that the bug is exclusive to Linux. From what I've seen, it's a fairly common trend for security vulnerabilities on Linux to also be applicable to Windows in some way, especially for similar low-level OS APIs. So unless we're confident that the issue is exclusive to Linux, I think it would be safer/better in the long term to apply the change platform independently. In the short term, it will likely be a minor inconvenience and maintenance cost to users explicitly passing `reuse_address=True`. But, I think it's worthwhile in this particular case, especially since the majority of users don't seem to be doing that (based on my earlier search results). |
|||
msg357177 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-21 15:33 | |
> A higher-level interface like asyncio doesn't necessarily want to map its kwargs directly to non-portable low-level options like this. Also reuse_port has portability issues, the whole portability mess i s nicely summed up in: https://stackoverflow.com/questions/14388706/how-do-so-reuseaddr-and-so-reuseport-differ And the docs say that "If the SO_REUSEPORT constant is not defined then this capability is unsupported." which is true but the converse is not - some platforms apparently do have SO_REUSEPORT defined but the option still doesn't work, resulting in a ValueError exception from create_datagram_endpoint(). So to create truly portable code, the developer who really wants to use these options has to know differences between underlying OS and kernel versions, catch errors that shouldn't happen according to docs and then try again with different options. This is ok for lower level APIs like socket.socket where things map 1:1 to system calls but not a good thing on higher level APIs as you point out. In my C++ projects that use these, I just have a #ifdef block for every OS and don't even try to abstract it. |
|||
msg357217 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-21 21:17 | |
> some platforms apparently do have SO_REUSEPORT defined but the option still doesn't work, resulting in a ValueError exception from create_datagram_endpoint(). Are you aware of what currently supported platforms have SO_REUSEPORT defined where the *reuse_port* parameter doesn't actually work? This would be quite helpful to know. AFAIK, it's available and supported on the majority of Unix platforms (such as Linux 3.9+, MacOS and BSD). On Windows, SO_REUSEPORT is undefined. Also, at the very least, the ValueError exception would occur is highly informative and specific: raise ValueError('reuse_port not supported by socket module, ' 'SO_REUSEPORT defined but not implemented.') > I just have a #ifdef block for every OS and don't even try to abstract it. The main issue with this approach for our purposes is that we don't directly support every single available platform, as making the code specific to an OS incurs a long-term maintenance cost. See https://www.python.org/dev/peps/pep-0011/ for more details. Indirect patches are fine, but we only add platform-specific code if it is officially supported: "Patches which add platform-specific code such as the name of a specific platform to the configure script will generally not be accepted without the platform having official support." IIUC, this applies to any code that explicitly defines the name of a specific platform. Official support not only involves us having someone with commit privileges that will provide it, but also a buildbot in place (or provided) and having the OS *version* supported upstream. For example, I think Linux kernel versions prior to 3.9 would fall under the category of "unsupported", since they are not supported upstream. |
|||
msg357295 - (view) | Author: Jukka Väisänen (vaizki) | Date: 2019-11-22 15:05 | |
> Are you aware of what currently supported platforms have SO_REUSEPORT defined where the *reuse_port* parameter doesn't actually work? This would be quite helpful to know. This was reported by a partner that was working porting our code to Android but might be fixed current Android API levels. I cannot test this myself. > .. we don't directly support every single available platform, as making the code specific to an OS incurs a long-term maintenance cost Fully agree, part of the reason why I was suggesting getting rid of reuse_address and reuse_port completely on this higher level API. But I'm happy with Antoine's proposal which effectively kills the reuse_address parameter, just suggesting we also kill reuse_port as well. |
|||
msg357335 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-11-22 23:12 | |
> This was reported by a partner that was working porting our code to Android but might be fixed current Android API levels. I cannot test this myself. Thanks, it's helpful to be aware of potential incompatibilities either way. I don't think we directly support or test Android, based on https://buildbot.python.org/all/#/builders. There's some degree of _indirect_ support because Android kernels are based on Linux LTS kernels, but there are substantial differences. > Fully agree, part of the reason why I was suggesting getting rid of reuse_address and reuse_port completely on this higher level API. > But I'm happy with Antoine's proposal which effectively kills the reuse_address parameter, just suggesting we also kill reuse_port as well. Removing support for the *reuse_address* parameter can be justified with the security issue, but I don't think we have significant justification to also remove *reuse_port* in the same PR. In general, removing a parameter is not something taken lightly because of the backwards compatibility concerns. I think the argument for eventually deprecating *reuse_port* on the basis of it directly mapping to a low-level OS API could be worth further discussion, but that's a topic for an entirely separate issue IMO. If it is approved, it would have to go through the usual deprecation process, likely starting in 3.9 with a DeprecationWarning and a removal no sooner than 3.11. |
|||
msg357377 - (view) | Author: Andrew Svetlov (asvetlov) * | Date: 2019-11-23 12:17 | |
I support Antoine's proposal. Let's remove the suspicious option if we cannot handle it correctly in a secured manner. If people still want to use SO_REUSEADDR they can apply it manually, old good transp.get_extra_info("socket") is still available. |
|||
msg357985 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-12-07 20:33 | |
Where are we with this? The deadline for 3.8.1 and 3.7.6 is coming up in a few days. |
|||
msg357990 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-07 23:32 | |
> Where are we with this? The deadline for 3.8.1 and 3.7.6 is coming up in a few days. I believe we're just waiting on review and additional feedback on GH-17311, which implements Antoine's proposal. The only remaining component I can think of is the "What's New" entry, but I was planning on doing that in a separate PR so that we could get the security fix out there first (and simplify the merge conflicts for the backports). |
|||
msg358101 - (view) | Author: Łukasz Langa (lukasz.langa) * | Date: 2019-12-09 14:21 | |
New changeset ab513a38c98695f271e448fe2cb7c5e39eeaaaaf by Łukasz Langa (Kyle Stanley) in branch 'master': bpo-37228: Fix loop.create_datagram_endpoint()'s usage of SO_REUSEADDR (#17311) https://github.com/python/cpython/commit/ab513a38c98695f271e448fe2cb7c5e39eeaaaaf |
|||
msg358104 - (view) | Author: Łukasz Langa (lukasz.langa) * | Date: 2019-12-09 14:39 | |
New changeset 79c29742a8ba96fc933efe34e55e90537b3eb780 by Łukasz Langa (Miss Islington (bot)) in branch '3.8': bpo-37228: Fix loop.create_datagram_endpoint()'s usage of SO_REUSEADDR (GH-17311) (#17529) https://github.com/python/cpython/commit/79c29742a8ba96fc933efe34e55e90537b3eb780 |
|||
msg358105 - (view) | Author: Łukasz Langa (lukasz.langa) * | Date: 2019-12-09 14:44 | |
Kyle, I'm releasing 3.8.1rc1 now. Please add the What's New entry before next Monday (3.8.1). |
|||
msg358162 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-10 05:04 | |
Thanks for taking care of merging to 3.x (master) and 3.8, Łukasz! > Kyle, I'm releasing 3.8.1rc1 now. Please add the What's New entry before next Monday (3.8.1). No problem, I'll definitely have time to do that before 3.8.1 final, likely in the next few days. I'll add you as a reviewer the to PR. After adding the What's New to 3.x and 3.8, I'll work on the manual backport PRs to 3.7, 3,6, and 3.5 via cherry-pick. This was suggested by Ned in the comments of GH-17311, for having a separate versionchanged for each of those branches. |
|||
msg358244 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-12-11 04:49 | |
New changeset 95157c6a281ccfc7a92a17dfb8d7b5338cad5cb7 by Ned Deily in branch '3.7': bpo-37228: Fix loop.create_datagram_endpoint()'s usage of SO_REUSEADDR (GH-17311) (GH-17570) https://github.com/python/cpython/commit/95157c6a281ccfc7a92a17dfb8d7b5338cad5cb7 |
|||
msg358245 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-12-11 04:54 | |
The backport to 3.7 seems straightforward so I did it to unblock 3.7.6rc1. The backport to 3.6 is a bit more complicated and 3.6.10rc1 can wait a bit longer so I'll leave that for Kyle along with the various What's New entries. |
|||
msg358246 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-11 05:39 | |
> The backport to 3.7 seems straightforward so I did it to unblock 3.7.6rc1. The backport to 3.6 is a bit more complicated and 3.6.10rc1 can wait a bit longer so I'll leave that for Kyle along with the various What's New entries. Thanks, Ned. I'll prioritize working on the What's New entries for 3.7 and 3.8 (likely in the same PR to master) before working on the backports to 3.6 and 3.5, since the release for 3.7.1 and 3.7.6 are coming up soon. |
|||
msg358247 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-11 05:42 | |
> since the release for 3.7.1 and 3.7.6 are coming up soon. Clarification: should be "since the release for 3.8.1 and 3.7.6 are coming up soon", that was a typo. |
|||
msg358248 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-12-11 05:44 | |
Actually, 3.6.10rc1 is currently blocked by this so if you do have time to work on it first, that would be great. |
|||
msg358249 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-11 05:47 | |
Oh okay, I'll work on the 3.6 backport first then. |
|||
msg358252 - (view) | Author: Ned Deily (ned.deily) * | Date: 2019-12-11 06:54 | |
New changeset b23c0840ce07e03f2705fb08d94f8f03e5c5d5b8 by Ned Deily (Kyle Stanley) in branch '3.6': [3.6] bpo-37228: Fix loop.create_datagram_endpoint()'s usage of SO_REUSEADDR (GH-17311). (GH-17571) https://github.com/python/cpython/commit/b23c0840ce07e03f2705fb08d94f8f03e5c5d5b8 |
|||
msg358254 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-11 07:08 | |
Now that the backports for 3.6-3.8 are merged, I'll work on the What's New entries next. Waiting on feedback from Larry Hastings regarding the potential 3.5 backport, I'll add him to the nosy list. |
|||
msg358288 - (view) | Author: Karthikeyan Singaravelan (xtreak) * | Date: 2019-12-12 05:54 | |
The fix seems to generate few DeprecationWarning while running test suite with -Wall. ➜ cpython git:(master) git checkout ab513a38c98695f271e448fe2cb7c5e39eeaaaaf~1 Lib/asyncio Updated 1 path from 4fb4056fbc ➜ cpython git:(master) ✗ ./python.exe -Wall -m test test_asyncio -m test_create_datagram_endpoint_nosoreuseport -m test_create_datagram_endpoint_ip_addr 0:00:00 load avg: 3.12 Run tests sequentially 0:00:00 load avg: 3.12 [1/1] test_asyncio == Tests result: SUCCESS == 1 test OK. Total duration: 133 ms Tests result: SUCCESS ➜ cpython git:(master) ✗ git checkout ab513a38c98695f271e448fe2cb7c5e39eeaaaaf Lib/asyncio Updated 1 path from 63d1437ba5 ➜ cpython git:(master) ./python.exe -Wall -m test test_asyncio -m test_create_datagram_endpoint_nosoreuseport -m test_create_datagram_endpoint_ip_addr 0:00:00 load avg: 3.03 Run tests sequentially 0:00:00 load avg: 3.03 [1/1] test_asyncio /Users/kasingar/stuff/python/cpython/Lib/asyncio/events.py:81: DeprecationWarning: The *reuse_address* parameter has been deprecated as of 3.5.10 and is scheduled for removal in 3.11. self._context.run(self._callback, *self._args) /Users/kasingar/stuff/python/cpython/Lib/asyncio/events.py:81: DeprecationWarning: The *reuse_address* parameter has been deprecated as of 3.5.10 and is scheduled for removal in 3.11. self._context.run(self._callback, *self._args) == Tests result: SUCCESS == 1 test OK. Total duration: 135 ms Tests result: SUCCESS |
|||
msg358289 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-12 06:08 | |
> The fix seems to generate few DeprecationWarning while running test suite with -Wall. Thanks Karthikeyan, I'm guessing that I missed an assertWarns() or left an outdated test somewhere that explicitly sets `reuse_address=False` (since `reuse_address=True` would raise a ValueError instead of a DeprecationWarning). I'll do some investigation and fix the issue as soon as possible. |
|||
msg358291 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-12 06:32 | |
> or left an outdated test somewhere that explicitly sets `reuse_address=False` Looks like this was the issue, I left a `reuse_address=False` in both test_create_datagram_endpoint_nosoreuseport and test_create_datagram_endpoint_ip_addr. I fixed it locally on the latest commit to master: [aeros:~/repos/cpython]$ ./python -Wall -m test test_asyncio -m test_create_datagram_endpoint_nosoreuseport -m test_create_datagram_endpoint_ip_addr 0:00:00 load avg: 0.29 Run tests sequentially 0:00:00 load avg: 0.29 [1/1] test_asyncio == Tests result: SUCCESS == 1 test OK. Total duration: 130 ms Tests result: SUCCESS I also used git grep to ensure `reuse_address=False` didn't exist at any other locations, other than the intentional one to test the DeprecationWarning. But while doing that, I did notice that `reuse_address=` is present in several other locations through Lib/asyncio, which will likely add some confusing deprecation warnings to anyone running their asyncio programs with -Wall. The way we did the deprecation was using an _unset sentinel, so even `reuse_address=None` will cause a DeprecationWarning (since the parameter will be removed entirely in 3.11). I'll fix them accordingly and open a new PR (as well as the backports). |
|||
msg358292 - (view) | Author: Karthikeyan Singaravelan (xtreak) * | Date: 2019-12-12 06:34 | |
One more resource warning about unclosed resource being garbage collected. As per the other tests I think transport and protocol need to be closed as per below patch but someone can verify if it's the right approach. ./python.exe -X tracemalloc -Wall -m test test_asyncio -m test_create_datagram_endpoint_reuse_address_warning 0:00:00 load avg: 2.45 Run tests sequentially 0:00:00 load avg: 2.45 [1/1] test_asyncio /Users/kasingar/stuff/python/cpython/Lib/test/support/__init__.py:1722: ResourceWarning: unclosed <socket.socket fd=6, family=AddressFamily.AF_INET, type=SocketKind.SOCK_DGRAM, proto=17, laddr=('127.0.0.1', 62353)> gc.collect() Object allocated at (most recent call last): File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/base_events.py", lineno 1331 sock = socket.socket( /Users/kasingar/stuff/python/cpython/Lib/asyncio/selector_events.py:694: ResourceWarning: unclosed transport <_SelectorDatagramTransport fd=6> _warn(f"unclosed transport {self!r}", ResourceWarning, source=self) Object allocated at (most recent call last): File "/Users/kasingar/stuff/python/cpython/Lib/asyncio/selector_events.py", lineno 84 return _SelectorDatagramTransport(self, sock, protocol, == Tests result: SUCCESS == 1 test OK. Total duration: 373 ms Tests result: SUCCESS diff --git Lib/test/test_asyncio/test_base_events.py Lib/test/test_asyncio/test_base_events.py index 6c0f00dc93..338f35c8dd 100644 --- Lib/test/test_asyncio/test_base_events.py +++ Lib/test/test_asyncio/test_base_events.py @@ -1814,7 +1814,9 @@ class BaseEventLoopWithSelectorTests(test_utils.TestCase): reuse_address=False) with self.assertWarns(DeprecationWarning): - self.loop.run_until_complete(coro) + transport, protocol = self.loop.run_until_complete(coro) + transport.close() + self.loop.run_until_complete(protocol.done) @patch_socket def test_create_datagram_endpoint_nosoreuseport(self, m_socket): After patch there are no resource warnings for the test. ./python.exe -X tracemalloc -Wall -m test test_asyncio -m test_create_datagram_endpoint_reuse_address_warning 0:00:00 load avg: 2.09 Run tests sequentially 0:00:00 load avg: 2.09 [1/1] test_asyncio == Tests result: SUCCESS == 1 test OK. Total duration: 349 ms Tests result: SUCCESS |
|||
msg358293 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-12 06:44 | |
> I'll fix them accordingly and open a new PR (as well as the backports). Nevermind. Upon further inspection, the other occurrences of `reuse_address=` were for create_server(), not create_datagram_endpoint(). The PR will only include the removal of the two occurrences of `reuse_address=False` in Lib/test/test_asyncio/test_base_events.py. Fortunately, this _should_ mean that the `needs backport to x` labels and automated backports from miss-islington will work correctly, since the changes are rather minimal. Thanks again for pointing it out Karthikeyan, as well as mentioning the specific tests the DeprecationWarnings were coming from. That made it a lot easier to figure out the source of the warnings. |
|||
msg358294 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-12 07:09 | |
> One more resource warning about unclosed resource being garbage collected. As per the other tests I think transport and protocol need to be closed as per below patch but someone can verify if it's the right approach. Ah, good catch. It's not needed when the ValueError occurs because it prevents the transport and protocol from being created in the first place, but I forgot that it would still be needed for the DeprecationWarning test. In addition to your changes, I'll also add a self.assertEqual('CLOSED', protocol.state). This is done in most of the other tests and it doesn't hurt to make sure the protocol properly closed. I'll add your fix to the PR and add you to the "Co-authored-by" for that commit, thanks. |
|||
msg358372 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-14 02:04 | |
Opened a PR that adds the whatsnew entries to master, 3.8, 3.7, and 3.6: GH-17595. |
|||
msg358531 - (view) | Author: Kyle Stanley (aeros) * | Date: 2019-12-17 04:17 | |
Thanks for taking care of merging the remaining backport PRs for 3.6-3.8, Ned. Now, the only branch left is (potentially) 3.5. |
|||
msg362636 - (view) | Author: Łukasz Langa (lukasz.langa) * | Date: 2020-02-25 12:09 | |
Downgrading priority since it's released everywhere except for 3.5. |
|||
msg394171 - (view) | Author: Ned Deily (ned.deily) * | Date: 2021-05-22 00:02 | |
Since 3.5 has now reached end-of-life, this issue will not be fixed there so it looks like it can be closed. |
|||
msg394185 - (view) | Author: Kyle Stanley (aeros) * | Date: 2021-05-22 18:53 | |
> Since 3.5 has now reached end-of-life, this issue will not be fixed there so it looks like it can be closed. Thanks, Ned <3 |
|||
msg394186 - (view) | Author: Kyle Stanley (aeros) * | Date: 2021-05-22 18:53 | |
> Thanks, Ned <3 (For following up and closing the issue) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:16 | admin | set | github: 81409 |
2021-05-22 18:53:33 | aeros | set | messages: + msg394186 |
2021-05-22 18:53:06 | aeros | set | messages: + msg394185 |
2021-05-22 00:02:37 | ned.deily | set | status: open -> closed versions: - Python 3.5 messages: + msg394171 resolution: fixed stage: patch review -> resolved |
2020-02-25 12:09:37 | lukasz.langa | set | priority: deferred blocker -> normal messages: + msg362636 |
2019-12-17 04:17:35 | aeros | set | messages: + msg358531 |
2019-12-16 22:50:44 | aeros | set | pull_requests: + pull_request17101 |
2019-12-16 22:50:06 | aeros | set | pull_requests: + pull_request17100 |
2019-12-16 22:49:10 | aeros | set | pull_requests: + pull_request17099 |
2019-12-14 02:04:57 | aeros | set | messages: + msg358372 |
2019-12-14 02:02:33 | aeros | set | pull_requests: + pull_request17067 |
2019-12-12 13:49:56 | miss-islington | set | pull_requests: + pull_request17055 |
2019-12-12 13:49:50 | miss-islington | set | pull_requests: + pull_request17054 |
2019-12-12 13:49:44 | miss-islington | set | pull_requests: + pull_request17053 |
2019-12-12 07:35:41 | aeros | set | pull_requests: + pull_request17051 |
2019-12-12 07:09:15 | aeros | set | messages: + msg358294 |
2019-12-12 06:44:33 | aeros | set | messages: + msg358293 |
2019-12-12 06:38:05 | gvanrossum | set | nosy:
- gvanrossum |
2019-12-12 06:34:23 | xtreak | set | messages: + msg358292 |
2019-12-12 06:32:52 | aeros | set | messages: + msg358291 |
2019-12-12 06:08:03 | aeros | set | messages: + msg358289 |
2019-12-12 05:54:03 | xtreak | set | nosy:
+ xtreak messages: + msg358288 |
2019-12-11 07:08:32 | aeros | set | nosy:
+ larry messages: + msg358254 |
2019-12-11 06:54:14 | ned.deily | set | messages: + msg358252 |
2019-12-11 06:21:40 | aeros | set | pull_requests: + pull_request17045 |
2019-12-11 05:47:26 | aeros | set | messages: + msg358249 |
2019-12-11 05:44:19 | ned.deily | set | messages: + msg358248 |
2019-12-11 05:42:39 | aeros | set | messages: + msg358247 |
2019-12-11 05:39:08 | aeros | set | messages: + msg358246 |
2019-12-11 04:54:08 | ned.deily | set | messages: + msg358245 |
2019-12-11 04:49:30 | ned.deily | set | messages: + msg358244 |
2019-12-11 04:29:08 | ned.deily | set | stage: patch review pull_requests: + pull_request17044 |
2019-12-10 05:04:19 | aeros | set | messages: + msg358162 |
2019-12-09 14:44:41 | lukasz.langa | set | messages:
+ msg358105 stage: patch review -> (no value) |
2019-12-09 14:39:58 | lukasz.langa | set | messages: + msg358104 |
2019-12-09 14:21:23 | miss-islington | set | stage: patch review pull_requests: + pull_request17007 |
2019-12-09 14:21:14 | lukasz.langa | set | nosy:
+ lukasz.langa messages: + msg358101 |
2019-12-07 23:32:04 | aeros | set | messages: + msg357990 |
2019-12-07 20:33:49 | ned.deily | set | messages: + msg357985 |
2019-11-23 12:17:08 | asvetlov | set | messages: + msg357377 |
2019-11-22 23:12:40 | aeros | set | messages: + msg357335 |
2019-11-22 15:05:46 | vaizki | set | messages: + msg357295 |
2019-11-21 21:17:31 | aeros | set | messages: + msg357217 |
2019-11-21 15:33:05 | vaizki | set | messages: + msg357177 |
2019-11-21 10:26:32 | aeros | set | messages: + msg357154 |
2019-11-21 09:00:39 | njs | set | messages: + msg357145 |
2019-11-21 08:47:18 | aeros | set | messages: + msg357143 |
2019-11-21 05:50:04 | gvanrossum | set | messages:
+ msg357131 title: UDP sockets created by create_datagram_endpoint() allow by default multiple processes to bind the same port -> UDP sockets created by create_datagram_endpoint() allow by default multiple processes to bind the same port |
2019-11-21 03:18:54 | yselivanov | set | messages:
+ msg357128 stage: patch review -> (no value) |
2019-11-21 03:16:10 | aeros | set | stage: patch review |
2019-11-21 03:15:17 | aeros | set | messages:
+ msg357127 stage: patch review -> (no value) |
2019-11-21 03:00:19 | aeros | set | keywords:
+ patch stage: patch review pull_requests: + pull_request16799 |
2019-11-21 02:39:28 | yselivanov | set | messages: + msg357126 |
2019-11-21 02:33:30 | aeros | set | messages: + msg357124 |
2019-11-21 02:19:24 | aeros | set | messages: + msg357123 |
2019-11-21 00:26:01 | ned.deily | set | priority: normal -> deferred blocker versions: + Python 3.5, Python 3.6, Python 3.7, Python 3.8 nosy: + ned.deily messages: + msg357115 type: behavior -> security |
2019-11-20 22:49:18 | vaizki | set | messages: + msg357109 |
2019-11-20 22:35:35 | aeros | set | messages: + msg357108 |
2019-11-20 21:04:18 | dacut | set | messages: + msg357103 |
2019-11-20 20:27:28 | dacut | set | messages: + msg357101 |
2019-11-20 12:25:15 | pitrou | set | messages: + msg357068 |
2019-11-20 12:23:04 | pitrou | set | messages: - msg357067 |
2019-11-20 12:21:21 | pitrou | set | nosy:
+ pitrou messages: + msg357067 |
2019-11-20 12:18:16 | vaizki | set | messages: + msg357066 |
2019-11-20 10:17:55 | aeros | set | nosy:
+ aeros messages: + msg357049 |
2019-11-20 09:21:02 | njs | set | messages: + msg357042 |
2019-11-20 08:59:03 | vaizki | set | messages: + msg357040 |
2019-11-20 08:26:07 | dacut | set | messages: + msg357039 |
2019-11-20 01:33:40 | dacut | set | messages: + msg357018 |
2019-11-20 00:33:12 | dacut | set | messages: + msg357011 |
2019-11-19 09:07:41 | vaizki | set | messages: + msg356956 |
2019-11-18 22:59:17 | dacut | set | nosy:
+ dacut messages: + msg356927 |
2019-11-18 21:15:10 | njs | set | messages: + msg356910 |
2019-11-18 20:59:07 | gvanrossum | set | nosy:
+ gvanrossum messages: + msg356909 |
2019-11-18 18:51:28 | vaizki | set | messages: + msg356897 |
2019-11-18 16:17:33 | yselivanov | set | nosy:
+ njs messages: + msg356876 |
2019-11-18 10:48:08 | vaizki | set | messages: + msg356856 |
2019-11-18 10:18:58 | asvetlov | set | messages: + msg356855 |
2019-11-18 10:00:33 | vaizki | set | messages:
+ msg356854 versions: + Python 3.9, - Python 3.7 |
2019-06-11 09:39:30 | asvetlov | set | messages: + msg345210 |
2019-06-11 09:36:54 | vaizki | create |