Re: [nmh-workers] closefds() _before_ fork?
Date:Tue, 23 Apr 2019 19:35:11 -0400 From:Ken Hornstein Message-ID: <20190423233511.ea88b143...@pb-smtp1.pobox.com> | There are tons of examples like this in nmh; plenty of child processes | are created with descriptors open. It is worth remembering that MH was written before close-on-exec was invented ... I suspect that much of what you're seeing is simply a result of that, rather than anything else in particular. Using close-on-exec sounds like the right way to me, and if it turns out that some particular uses require a fd to be passed down (which usually also requires some arg to tell the child which fd that is) then those can be fixed when discovered. kre -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
[nmh-workers] Content caching code
As long as we're getting rid of stuff ... Does anyone use the content caching code? (See mhstore(1) for details). First, it turns out that when mhn was split into mhstore, mhshow, and mhlist, there was never a call to cache_all_messages() (the entry point into mhcachesbr.c) put in those individual programs, so in essence it would only ever store something in a cache if you called mhn (which has been deprecated for approximately forever). There are some calls to find_cache() in mhparse.c, but since there is nothing in the normal case that stores something in the cache it would never be used. Also, the security properties of a public cache seem pretty scary to me in this day and age (if you used one, and since the code mostly seems to not work I doubt anyone does, but I've been proven wrong before). I propose we get rid of it all; clearly no one has missed it. Thoughts? --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] Removing message/partial support
>I can't remember if I followed up or not, but over on the ietf-822 list >there were 3 or 4 products mentioned that had code but shipped with >the ability configured disabled. Gnus seems to be the largest user >base that would potentially handle a message/partial either incoming or >outbound. This implies "It's only of use if you already know a priori >that your recipient is running an uncommon mail stack". You didn't mention it, but I went over and looked. When the only one that seems to ship with the ability do it is Gnus, then I think that tells me everything I need to know. I'm going to yank this all out unless someone can come up with a plausible reason to keep it. (And I see Ned weighed in on the base64 discussion we had a while ago; I interpret his response as "it's perfectly fine to simply just stop decoding at that point, emitting a warning is recommended but not required"). --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>> to that end, i propose that we treat any open descriptor N>2 at the >> time of an exec() to be a bug, which is to be fixed by setting >> O_CLOEXEC at time of creation. > >What impact will such a decision have on tools like setlock which open a >filedescriptor, obtain an exclusive lock, and then exec a process? For >example, I may use something like the following in a script to prevent >multiple processes from incorporating email: > >setlock /tmp/mit.lock inc +MyIncTmp ... Well, my reading of the setlock man page (assuming this is the FreeBSD one) is that it is not depending on a _descriptor_ to be passed down (I don't even know how that would work), but in fact it is opening a particular _file_ and locking it. But let's pretend that it actually did need to have a descriptor passed down to nmh programs, and the locking needed to extend to any children that inc forked (that's the critical case here). Today, depending on which code path was executed, that would either a) work fine (because we don't close any descriptors) or b) fail miserably (because we close all descriptors above 2, but sometimes maybe 3). My change makes it so we no longer close all descriptors when creating a child process (in the majority of cases we didn't). The particular change I have started implementing is that any descriptors _created by nmh_ will be marked as close-on-exec, so they will no longer be available to child processes (well, technically, child processes that have called exec(), but you know what I mean). For example, let's say inc(1) is talking to a POP server. Before, the network connection to the POP server was available to programs that inc(1) happened to fork off. One possible example would be the add-hook program if you happened to define one (the hook code never closed extra file descriptors). I think most people would agree that this is probably not desirable. Now that the network descriptors are marked as close-on-exec, child processes (like the hooks code) never see them. Even if the hooks are not doing anything malicious, it's easy to imagine possible problems with a network socket ending up in their address space. If the hooks code forks off a long-running process, for example, that could leave a network socket open that nmh had considered closed and could potentially cause problems if (for example) the POP server expected you to close the connection before it allowed another one. There are tons of examples like this in nmh; plenty of child processes are created with descriptors open. Most of these are for files, but clearly at best that's sloppy and at worst is dangerous. I can see this only getting worse if we get IMAP support or additional plugin interfaces. My eventual goal is to make it so every new descriptor created _by nmh_ has close-on-exec set by default. This wouldn't affect descriptors inherited by nmh programs (because they wouldn't know about them) or descriptors created by library functions (nmh wouldn't know about them either); hopefully any descriptors created internally by a library function would be marked properly (a quick check shows that this is not an unfounded assumption; at least on MacOS X the descriptor created by getutxent() is marked as close-on-exec). As a side note, I see that documentation for the hooks interface has never made it into a man page; anyone willing to rectify that? --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
On Wed, 24 Apr 2019 03:46:04 +0700, Robert Elz said: > fd is also made to be fd 3. > > If it is good enough to be stdin, it is good enough to be 3 as well, > if there is (or once was) some reason this is important. Derp. -ENOCAFFEINE :) -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Thus said Paul Vixie on Tue, 23 Apr 2019 05:59:05 -0700: > to that end, i propose that we treat any open descriptor N>2 at the > time of an exec() to be a bug, which is to be fixed by setting > O_CLOEXEC at time of creation. What impact will such a decision have on tools like setlock which open a filedescriptor, obtain an exclusive lock, and then exec a process? For example, I may use something like the following in a script to prevent multiple processes from incorporating email: setlock /tmp/mit.lock inc +MyIncTmp ... Thanks, Andy -- TAI64 timestamp: 40005cbf7b26 -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Date:Tue, 23 Apr 2019 14:39:48 -0400 From:"Valdis Kl=?utf-8?Q?=c4=93?=tnieks" Message-ID: <1.1556044788@turing-police> | The point is that the fragment of code doesn't actually *know* "it happened | to be what is wanted on stdin". For all this code knows, it's a dangling fd | that was opened by some library function to talk to dbus or something. ?? fd is deliberately being made stdin (I did not look at the code to see where that comes from, but I can only assume that it comes from somewhere). fd is also made to be fd 3. If it is good enough to be stdin, it is good enough to be 3 as well, if there is (or once was) some reason this is important. kre -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
[nmh-workers] Removing message/partial support
As discussed here: https://lists.gnu.org/archive/html/nmh-workers/2019-02/msg00035.html Currently our message/partial support does not work. I believe fixing it is straightforward, but given the links that David posted in that thread and my own research, almost no other MUAs support it and it is probably blocked by most mail providers due to concerns about viruses and spam. Can anyone think of a reason to keep it? I suggest removing all support for it, on both sender and receiver sides. --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Date:Tue, 23 Apr 2019 11:15:05 -0400 From:"Valdis Kl=?utf-8?Q?=c4=93?=tnieks" Message-ID: <4344.1556032505@turing-police> | The *real* WTF there is that the code doesn't actually know if fd 3 is | known to be open on a file that should, or if it's just a stray leaked fd. I don't know why fd 3 was wanted to be a dup of stdin, but aside from that oddity, there's notthing remarkable about the code (no WTF), the test is just avoiding a (pointless) dup2(3,3). Whatever fd 3 was before executing this code, it is closed after it unless it happened to be what is wanted on stdin. kre -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>Ken Hornstein writes: >> I realize that dup2() clears the FD_CLOEXEC flag on the new file >> descriptor so the "normal" case of an opened file being dup2() down to 0 >> would work correctly, but the wrinkle is that it does NOT if the old and >> new file descriptor are the same. That is admittedly unlikely, but it >> could happen in a few cases so I'd like to be as robust as possible. > >But is that really an issue? It'd only occur if you're passing down >your own stdin, which presumably you inherited without FD_CLOEXEC. Weeelll ... here's a hypothetical issue, which I could see happening. - Something gets invoked with stdin closed; note that many of these programs that this is an issue with are run unattended, e.g. slocal(1). So it's possible (but unlikely) that they could be run without a valid stdin. - A file is opened and gets assigned the lowest valid file descriptor, which in this case is 0. Because of the "new" policy (which we haven't implemented yet), we mark it with FD_CLOEXEC. - We want to pass this file descriptor to the stdin of a subprocess. So we end up calling dup2(0, 0). We would normally expect FD_CLOEXEC to be cleared, but in this specific case it is not. So when the child process is exec()d, it's standard input is closed (and presumably doesn't work properly). If there's one thing I've learned from nmh, it is "expect weird stuff to happen" :-) --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Ken Hornstein writes: > I realize that dup2() clears the FD_CLOEXEC flag on the new file > descriptor so the "normal" case of an opened file being dup2() down to 0 > would work correctly, but the wrinkle is that it does NOT if the old and > new file descriptor are the same. That is admittedly unlikely, but it > could happen in a few cases so I'd like to be as robust as possible. But is that really an issue? It'd only occur if you're passing down your own stdin, which presumably you inherited without FD_CLOEXEC. regards, tom lane -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>> I realize that dup2() clears the FD_CLOEXEC flag on the new file >> descriptor so the "normal" case of an opened file being dup2() down to >> 0 would work correctly, but the wrinkle is that it does NOT if the old >> and new file descriptor are the same. That is admittedly unlikely, >> but it could happen in a few cases so I'd like to be as robust as >> possible. > >Are you aware of dup3(2)? Hm, I had to poke around for that one. I see: dup3() is Linux-specific. (I'm aware that it seems to have made it to other operating systems, but I don't think we can rely on it). Also, it seems like dup3() doesn't do what I want, exactly. Specifically: If oldfd equals newfd, then dup3() fails with the error EINVAL. What I would like is a dup4(), where even if oldfd == newfd, then the close-on-exec flag is cleared. Not really a huge issue; we can work around it. --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Hi Ken, > I realize that dup2() clears the FD_CLOEXEC flag on the new file > descriptor so the "normal" case of an opened file being dup2() down to > 0 would work correctly, but the wrinkle is that it does NOT if the old > and new file descriptor are the same. That is admittedly unlikely, > but it could happen in a few cases so I'd like to be as robust as > possible. Are you aware of dup3(2)? -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>> It's not the only one, e.g. docs/historical/mh-6.8.5/uip/post.c has
>>
>> 2622 if (fd != 0)
>> 2623 (void) dup2 (fd, 0);
>> 2624 (void) freopen ("/dev/null", "w", stdout);
>> 2625 (void) freopen ("/dev/null", "w", stderr);
>> 2626 if (fd != 3)/* backwards compatible... */
>> 2627 (void) dup2 (fd, 3);
>> 2628 closefds (4);
>>
>> Note the comment.
>
>The *real* WTF there is that the code doesn't actually know if fd 3 is
>known to be open on a file that should, or if it's just a stray leaked fd.
Wow. So do any greybeards out there have any idea of why this is there??
Clearly this was a thing, but I have no idea why. I see that same code
in MH 5, so it's been that way for approximately forever.
--Ken
--
nmh-workers
https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>> I agree with the general principle that if we open it, we track it, and >> then close it so it doesn't reach the child, typically with O_CLOEXEC or >> FD_CLOEXEC. ... > >to that end, i propose that we treat any open descriptor N>2 at the time >of an exec() to be a bug, which is to be fixed by setting O_CLOEXEC at >time of creation. +1. There are a few sneaky spots where we have to be careful; sometimes a file is opened and then expected to be used as stdin for a child process; there is also the unusual case of the -idanno flag, and it wouldn't surprise me if there was another use of a file descriptor passed down to a child process that I didn't know about. I realize that dup2() clears the FD_CLOEXEC flag on the new file descriptor so the "normal" case of an opened file being dup2() down to 0 would work correctly, but the wrinkle is that it does NOT if the old and new file descriptor are the same. That is admittedly unlikely, but it could happen in a few cases so I'd like to be as robust as possible. It seems like the right way forward, given that nowadays there are a lot of spots where we could fork another process off, is that every descriptor we open should have FD_CLOEXEC set (I added that to the networking code). Considering we haven't done that in the past for a lot of cases it doesn't seem like there's a lot of urgency, but we'll file this under "ongoing cleanup" --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
On Tue, 23 Apr 2019 13:35:44 +0100, Ralph Corderoy said:
> It's not the only one, e.g. docs/historical/mh-6.8.5/uip/post.c has
>
> 2622 if (fd != 0)
> 2623 (void) dup2 (fd, 0);
> 2624 (void) freopen ("/dev/null", "w", stdout);
> 2625 (void) freopen ("/dev/null", "w", stderr);
> 2626 if (fd != 3)/* backwards compatible... */
> 2627 (void) dup2 (fd, 3);
> 2628 closefds (4);
>
> Note the comment.
The *real* WTF there is that the code doesn't actually know if fd 3 is
known to be open on a file that should, or if it's just a stray leaked fd.
--
nmh-workers
https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>> they called getdtablesize() on Linux, which it seems returns a smaller >> number than getrlimit(). > >That's surprising. I thought getdtablesize() was effectively > > return getrlimit(RLIMIT_NOFILE, ) < 0 ? OPEN_MAX : ru.rlim_cur; Hey, I don't make the news, I just report it. If you look at the bug fix referenced in that thread, the "fix" was to make sure that Linux wasn't detected as SVR4. That makes it so it calls getdtablesize() instead of getrlimit(). My understanding of getdtablesize() matches yours, but I can't see how that "fix" could make this problem better otherwise. --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Hi Ken, > they called getdtablesize() on Linux, which it seems returns a smaller > number than getrlimit(). That's surprising. I thought getdtablesize() was effectively return getrlimit(RLIMIT_NOFILE, ) < 0 ? OPEN_MAX : ru.rlim_cur; That seems to match /lib/libc.a here, if I squint a bit, assuming OPEN_MAX is 256. <__getdtablesize>: 0: f3 0f 1e fa endbr64 4: 48 83 ec 28 sub$0x28,%rsp 8: bf 07 00 00 00 mov$0x7,%edi d: 64 48 8b 04 25 28 00mov%fs:0x28,%rax 14: 00 00 16: 48 89 44 24 18 mov%rax,0x18(%rsp) 1b: 31 c0 xor%eax,%eax 1d: 48 89 e6mov%rsp,%rsi 20: e8 00 00 00 00 callq 25 <__getdtablesize+0x25> 21: R_X86_64_PLT32 __getrlimit-0x4 25: 8b 14 24mov(%rsp),%edx 28: 85 c0 test %eax,%eax 2a: 78 1c js 48 <__getdtablesize+0x48> 2c: 48 8b 4c 24 18 mov0x18(%rsp),%rcx 31: 64 48 33 0c 25 28 00xor%fs:0x28,%rcx 38: 00 00 3a: 89 d0 mov%edx,%eax 3c: 75 11 jne4f <__getdtablesize+0x4f> 3e: 48 83 c4 28 add$0x28,%rsp 42: c3 retq 43: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) 48: ba 00 01 00 00 mov$0x100,%edx 4d: eb dd jmp2c <__getdtablesize+0x2c> 4f: ff 15 00 00 00 00 callq *0x0(%rip)# 55 <__getdtablesize+0x55> 51: R_X86_64_GOTPCRELX __stack_chk_fail-0x4 -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
>I agree with the general principle that if we open it, we track it, and >then close it so it doesn't reach the child, typically with O_CLOEXEC or >FD_CLOEXEC. Calling close(2) lots of times based on getdtablesize(3) >can take a while. screen(1) had a bug recently where it was taking ages >trying to close almost 512 Ki of them. >https://lists.archlinux.org/pipermail/arch-general/2019-March/046214.html I was curious what they did, so I looked ... it looks like they actually didn't really "solve" the issue. They use a function fdwalk() on Solaris but made sure they called getdtablesize() on Linux, which it seems returns a smaller number than getrlimit(). --Ken -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Ralph Corderoy wrote on 2019-04-23 05:10: ... I agree with the general principle that if we open it, we track it, and then close it so it doesn't reach the child, typically with O_CLOEXEC or FD_CLOEXEC. ... to that end, i propose that we treat any open descriptor N>2 at the time of an exec() to be a bug, which is to be fixed by setting O_CLOEXEC at time of creation. -- P Vixie -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Hi Ken,
> but ALSO uses dup2() to make an additional copy of the input file
> descriptor to descriptor 3 (!). Does anyone know why? It looks like
> it has always done this.
It's not the only one, e.g. docs/historical/mh-6.8.5/uip/post.c has
2622 if (fd != 0)
2623 (void) dup2 (fd, 0);
2624 (void) freopen ("/dev/null", "w", stdout);
2625 (void) freopen ("/dev/null", "w", stderr);
2626 if (fd != 3)/* backwards compatible... */
2627 (void) dup2 (fd, 3);
2628 closefds (4);
Note the comment.
--
Cheers, Ralph.
--
nmh-workers
https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] closefds() _before_ fork?
Hi, David wrote: > Ken wrote: > > I suggest we simply remove closefds() completely. ... > Ralph's suggestion of looking for only open fds addresses the > suboptimality of closefds(), though only for systems that use /proc. That was just meant as a temporary debugging aid to see what was sneaking through. :-) I agree with the general principle that if we open it, we track it, and then close it so it doesn't reach the child, typically with O_CLOEXEC or FD_CLOEXEC. Calling close(2) lots of times based on getdtablesize(3) can take a while. screen(1) had a bug recently where it was taking ages trying to close almost 512 Ki of them. https://lists.archlinux.org/pipermail/arch-general/2019-March/046214.html I don't think there's any multi-threading in nmh so we just have what's open at the time of fork(2) to deal with. -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] send fails
Hi David, > > > send: -port 2525 -alias /home/wilson/.mh_aliases -server mail.eskimo.com > > > > Yes, that would work, though only for you and not any other users. > > The port can't be set in mts.conf, so it wouldn't be straightforward > to support that for all users. No, agreed. But I snipped too much context and Stewart had worked out it was the -server that was key to success so I *think* that was the `that' to which I referred. -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
Re: [nmh-workers] Handling Configuration-file Read Errors.
Hi Ken, > I don't believe mts.conf is technically required, but I hear you. > So you're fine with erroring out if opening MHMTSCONF fails for any > reason ... and for the default mts.conf file, would you rather error > out if errno != ENOENT, or if errno == EACCES, or some other > condition? Taking each configuration file, at the level of system or user, and deciding if it's optional, based on existing documentation and code, and whether that's correct, seems orthogonal to how to handle problems with optional and mandatory configuration files. So I was punting on that with https://savannah.nongnu.org/bugs/?56192 :-) mts.conf and environment-variable friends was just the case at hand. Getting agreement on the desired handling, and I think we've got that, means we can then check how reality matches per configuration file. -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers
