* shell needs to change fd in a FILE @ 2017-07-31 15:05 Denys Vlasenko 2017-07-31 20:18 ` Rich Felker 0 siblings, 1 reply; 5+ messages in thread From: Denys Vlasenko @ 2017-07-31 15:05 UTC (permalink / raw) To: musl, Rich Felker Hi, I'm using ordinary FILE i/o for reading scripts in hush shell, instead of rolling my own implementation, so that I can reuse buffering code in libc, through the use of fgetc(). This works for almost all cases, except this one: if in script I have a redirect which says shell wants to open a fd which happens to be equal to the fd (say, 10) shell already used for script FILE: exec 10>FILE What other shells do in this situation is they simply dup and close script fd [in real code, they use fcntl(F_DUPFD) instead of dup() since they want to avoid getting low fds], so that fd is "moved" and no longer collides with the redirect. I can do this trick, but since I use FILE interface, then I need to inform libc that it needs to use new fd for this FILE. "fileno(fp) = new_fd;" is non-portable and does not work in either musl or glibc: it's a function, not a macro referencing (fp)->field_holding_fd. "fclose(fp); fp = fdopen(new_fd);" is not good since fp may have some buffered input, which will be lost by such code. How about adding a "set_fileno(fp, fd)" extension to musl, with some easy define to probe for to conditionally use it? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: shell needs to change fd in a FILE 2017-07-31 15:05 shell needs to change fd in a FILE Denys Vlasenko @ 2017-07-31 20:18 ` Rich Felker 2017-08-02 12:08 ` Denys Vlasenko 0 siblings, 1 reply; 5+ messages in thread From: Rich Felker @ 2017-07-31 20:18 UTC (permalink / raw) To: musl On Mon, Jul 31, 2017 at 05:05:24PM +0200, Denys Vlasenko wrote: > Hi, > > I'm using ordinary FILE i/o for reading scripts in hush shell, > instead of rolling my own implementation, so that I can reuse > buffering code in libc, through the use of fgetc(). > > This works for almost all cases, except this one: if in script > I have a redirect which says shell wants to open a fd which happens > to be equal to the fd (say, 10) shell already used for script FILE: > > exec 10>FILE > > What other shells do in this situation is they simply > dup and close script fd [in real code, they use fcntl(F_DUPFD) > instead of dup() since they want to avoid getting low fds], > so that fd is "moved" and no longer collides with the redirect. The right solution to this problem is not to actually reassign fds in the shell process at all, but instead reassign them in the child process to match their nominal (per the redirection operator) value. This is easy with fork+exec, hard or impossible to do safely with vfork+exec, and easy with posix_spawn. For commands which are internal (no child process), then, the nominal fd number is not the actual fd number but it doesn't matter; the internal logic can just remap it. Not saying you have to do it this way, but it's the clean (and the only strictly-conforming, since POSIX allows implementation-internal fd use that you *can't* safely move). > I can do this trick, but since I use FILE interface, then > I need to inform libc that it needs to use new fd for this FILE. > > "fileno(fp) = new_fd;" is non-portable and does not work in either > musl or glibc: it's a function, not a macro referencing > (fp)->field_holding_fd. > > "fclose(fp); fp = fdopen(new_fd);" is not good since fp may have > some buffered input, which will be lost by such code. I don't see how this is a problem unless you can read scripts from a non-seekable stream, which sounds really dubious. If the stream is seekable, you'll suffer a minor efficiency cost in this rare case, but it's not going to make a measurable difference in the big picture. > How about adding a "set_fileno(fp, fd)" extension to musl, > with some easy define to probe for to conditionally use it? I really don't want to get into the business of doing application-specific stdio extensions on request. We did a couple for gnulib, but that's because it's already crept into everything and the alternative was them doing awful hacks trying to poke at stdio internals. I think we can come up with a better solution here. Rich ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: shell needs to change fd in a FILE 2017-07-31 20:18 ` Rich Felker @ 2017-08-02 12:08 ` Denys Vlasenko 2017-08-02 15:38 ` Rich Felker 0 siblings, 1 reply; 5+ messages in thread From: Denys Vlasenko @ 2017-08-02 12:08 UTC (permalink / raw) To: musl On Mon, Jul 31, 2017 at 10:18 PM, Rich Felker <dalias@libc.org> wrote: > On Mon, Jul 31, 2017 at 05:05:24PM +0200, Denys Vlasenko wrote: >> Hi, >> >> I'm using ordinary FILE i/o for reading scripts in hush shell, >> instead of rolling my own implementation, so that I can reuse >> buffering code in libc, through the use of fgetc(). >> >> This works for almost all cases, except this one: if in script >> I have a redirect which says shell wants to open a fd which happens >> to be equal to the fd (say, 10) shell already used for script FILE: >> >> exec 10>FILE >> >> What other shells do in this situation is they simply >> dup and close script fd [in real code, they use fcntl(F_DUPFD) >> instead of dup() since they want to avoid getting low fds], >> so that fd is "moved" and no longer collides with the redirect. > > The right solution to this problem is not to actually reassign fds in > the shell process at all, but instead reassign them in the child > process to match their nominal (per the redirection operator) value. > This is easy with fork+exec, hard or impossible to do safely with > vfork+exec, and easy with posix_spawn. > > For commands which are internal (no child process), then, the nominal > fd number is not the actual fd number but it doesn't matter; the > internal logic can just remap it. "Just remap it" is actually quite a PITA in this case. This means "can't use standard functions emitting messages to stderr, because my stderr in builtins may be not on fd 2 but somewhere else". Would you enjoy this turn of events? It's not like shell logic is such an easy piece of code before you add this fd remapping thing. In busybox, echo, printf, kill, test shell builtins reuse code with standalone (non-shell) tools. Now all of those also can't use stdin and stderr as usual, they need to account for the possibility of remapped fds. Apart from builtins, busubox has these optional so-called NOFORK applets: they won't fork/exec/waitpid when run by shell, they simply call <applet>_main(). There are 25 NOFORKs. For example, printenv, pwd, whoami - these all output stuff to stdout. Many of them also can emit error messages. Now all of them also can't use stdin and stderr with usual functions. This is too WAY too much uglification. > Not saying you have to do it this way, but it's the clean (and the > only strictly-conforming, since POSIX allows implementation-internal > fd use that you *can't* safely move). For the functions used in shell, there is no reason for libc to use hidden fds. Shells don't use openlog() or DNS resolution functions. So this is only a theoretical thing to worry in this case. >> I can do this trick, but since I use FILE interface, then >> I need to inform libc that it needs to use new fd for this FILE. >> >> "fileno(fp) = new_fd;" is non-portable and does not work in either >> musl or glibc: it's a function, not a macro referencing >> (fp)->field_holding_fd. >> >> "fclose(fp); fp = fdopen(new_fd);" is not good since fp may have >> some buffered input, which will be lost by such code. > > I don't see how this is a problem unless you can read scripts from a > non-seekable stream, which sounds really dubious. I just tried: "bash /dev/tty" and "echo uname | bash /dev/fd/0" works. I prefer to not needlessly prevent my shell from being able to do that too. >> How about adding a "set_fileno(fp, fd)" extension to musl, >> with some easy define to probe for to conditionally use it? > > I really don't want to get into the business of doing > application-specific stdio extensions on request. We did a couple for > gnulib, but that's because it's already crept into everything and the > alternative was them doing awful hacks trying to poke at stdio > internals. I think we can come up with a better solution here. One easy solution is to ditch FILE and use my own buffering. ash/dash already did exactly that many years ago, for this very reason. This would mean that libc has some 25 kb of really good, well-debugged I/O buffering code which I am not using because it misses just one (!) trivial operation in the API, fp->fd = new_fd. This was the thought which prompted me to write the email: by seeing how much this would help, you might agree. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: shell needs to change fd in a FILE 2017-08-02 12:08 ` Denys Vlasenko @ 2017-08-02 15:38 ` Rich Felker 2017-08-02 16:02 ` Denys Vlasenko 0 siblings, 1 reply; 5+ messages in thread From: Rich Felker @ 2017-08-02 15:38 UTC (permalink / raw) To: musl On Wed, Aug 02, 2017 at 02:08:09PM +0200, Denys Vlasenko wrote: > On Mon, Jul 31, 2017 at 10:18 PM, Rich Felker <dalias@libc.org> wrote: > > On Mon, Jul 31, 2017 at 05:05:24PM +0200, Denys Vlasenko wrote: > >> Hi, > >> > >> I'm using ordinary FILE i/o for reading scripts in hush shell, > >> instead of rolling my own implementation, so that I can reuse > >> buffering code in libc, through the use of fgetc(). > >> > >> This works for almost all cases, except this one: if in script > >> I have a redirect which says shell wants to open a fd which happens > >> to be equal to the fd (say, 10) shell already used for script FILE: > >> > >> exec 10>FILE > >> > >> What other shells do in this situation is they simply > >> dup and close script fd [in real code, they use fcntl(F_DUPFD) > >> instead of dup() since they want to avoid getting low fds], > >> so that fd is "moved" and no longer collides with the redirect. > > > > The right solution to this problem is not to actually reassign fds in > > the shell process at all, but instead reassign them in the child > > process to match their nominal (per the redirection operator) value. > > This is easy with fork+exec, hard or impossible to do safely with > > vfork+exec, and easy with posix_spawn. > > > > For commands which are internal (no child process), then, the nominal > > fd number is not the actual fd number but it doesn't matter; the > > internal logic can just remap it. > > "Just remap it" is actually quite a PITA in this case. That's why I said: > > Not saying you have to do it this way, but it's the clean (and the It's just how I would go about implementing a shell, and the only way I'm aware of that's possible in a portable and hacks-free way. > This means "can't use standard functions emitting messages to stderr, > because my stderr in builtins may be not on fd 2 but somewhere else". > Would you enjoy this turn of events? It's not like shell logic is > such an easy piece of code before you add this fd remapping thing. I see. So if you have builtin commands executed with fd 2 redirected, and they potentially output errors, it would go to the wrong place. If all the messages are via explicit writes to stderr, this is probably not too bad to work around just by using a different FILE that can vary, but if things like perror also get used it could be a lot more of a pain. Initial adaptation might be fairly easy but avoiding regressions from inadvertent reintroduction of stderr access is not so easy. If you (or someone else) wanted to go this way, GCC's #pragma poison would probably be the tool for avoiding regressions -- poison all functions that implicitly use stderr. > > Not saying you have to do it this way, but it's the clean (and the > > only strictly-conforming, since POSIX allows implementation-internal > > fd use that you *can't* safely move). > > For the functions used in shell, there is no reason for libc to use > hidden fds. Shells don't use openlog() or DNS resolution functions. > So this is only a theoretical thing to worry in this case. I agree this is a more theoretical concern as long as you're happy with just working on Linux or very similar systems. In general, though, even things like the working directory could be implemented as a hidden fd. > >> I can do this trick, but since I use FILE interface, then > >> I need to inform libc that it needs to use new fd for this FILE. > >> > >> "fileno(fp) = new_fd;" is non-portable and does not work in either > >> musl or glibc: it's a function, not a macro referencing > >> (fp)->field_holding_fd. > >> > >> "fclose(fp); fp = fdopen(new_fd);" is not good since fp may have > >> some buffered input, which will be lost by such code. > > > > I don't see how this is a problem unless you can read scripts from a > > non-seekable stream, which sounds really dubious. > > I just tried: "bash /dev/tty" and "echo uname | bash /dev/fd/0" > works. I prefer to not needlessly prevent my shell from being able > to do that too. Is this just a theoretical concern or something you've encountered in the wild? FWIW I noticed a while back (and got bit badly) by hush's failure to safely remap the fd it's reading the script from, so I think replacing "fails badly as soon as the script exceeds stdio buffer size" with "just fails when using non-seekable script input" would be a significant improvement already. BTW one possible fix here would be checking whether the script input is seekable, and if it's not, disabling buffering. It would make execution of scripts from non-seekable input a lot more costly, but would always work. > >> How about adding a "set_fileno(fp, fd)" extension to musl, > >> with some easy define to probe for to conditionally use it? > > > > I really don't want to get into the business of doing > > application-specific stdio extensions on request. We did a couple for > > gnulib, but that's because it's already crept into everything and the > > alternative was them doing awful hacks trying to poke at stdio > > internals. I think we can come up with a better solution here. > > One easy solution is to ditch FILE and use my own buffering. > ash/dash already did exactly that many years ago, for this very reason. > > This would mean that libc has some 25 kb of really good, well-debugged > I/O buffering code which I am not using because it misses > just one (!) trivial operation in the API, fp->fd = new_fd. Poking through an abstraction to change internal state in a way that nobody has specified or thought out is trivial to implement, but not necessarily trivial to deal with the consequences of. Let me give a subtle example from somewhere different: malloc_usable_size(): You'd expect this function to be pretty simple and non-costly to support. The implementation has to know how much memory it allocated for a block in order to free it, so you'd think malloc_usable_size would not impose any implementation burden to store additional information about the allocation. Unfortunately, that's not correct. Suppose you call malloc(19) and, due to alignment issues, the implementation actually has to have the allocation end at 24 bytes rather than just 19. Now, if malloc_usable_size returns 24, the caller can assume it got lucky and can use indices 19-23 before having to realloc to get more, right? Nope. Unfortunately, the actual allocation size of 19 may be visible to the compiler (_FORTIFY_SOURCE/__builtin_object_size, and/or UBSan/ASan/etc.) at the point indices 19-23 are accessed, resulting in a trap for the (UB) accesses after malloc_usable_size told the caller it was okay to access them! So now we're stuck with a nonstandard hackish function we should never have implemented, malloc_usable_size, and need to find a place for the implementation to store the actual nominal size (19) in addition to the aligned size (24) just so that malloc_usable_size can return the right number to the caller to keep it from invoking UB. This will either require wasted space (extra field) or wasted time in standard codepaths (storing a non-aligned size and rounding it up every time malloc needs to access it internally). Bleh. I don't necessarily see something like this happening with set_fileno, but the whole point is that we didn't see it happening with malloc_usable_size either, until it did. > This was the thought which prompted me to write the email: > by seeing how much this would help, you might agree. I do see how helpful it would be, but there are also very good reasons I'm cautious (and the musl community tends to be very skeptical) of this sort of thing, which I hope you can understand too. Do you know what portion of the ~25k of buffered IO code (stdio) from musl you're actually using for parsing scripts? If it's mostly just getc and ungetc (or rather, if it doesn't involve complex operations like scanf) I think just using a separate light buffered-read framework makes the most sense. Yes there's a lot of value in musl's "really good, well-debugged" stdio code, but most of that value is in handling the full range of stdio behavior including subtle corner cases correctly, and isn't something you really get a lot of advantage from when you're just doing buffered sequential reading. Rich ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: shell needs to change fd in a FILE 2017-08-02 15:38 ` Rich Felker @ 2017-08-02 16:02 ` Denys Vlasenko 0 siblings, 0 replies; 5+ messages in thread From: Denys Vlasenko @ 2017-08-02 16:02 UTC (permalink / raw) To: musl On Wed, Aug 2, 2017 at 5:38 PM, Rich Felker <dalias@libc.org> wrote: > On Wed, Aug 02, 2017 at 02:08:09PM +0200, Denys Vlasenko wrote: >> On Mon, Jul 31, 2017 at 10:18 PM, Rich Felker <dalias@libc.org> wrote: >> > On Mon, Jul 31, 2017 at 05:05:24PM +0200, Denys Vlasenko wrote: > I see. So if you have builtin commands executed with fd 2 redirected, > and they potentially output errors, it would go to the wrong place. If > all the messages are via explicit writes to stderr, this is probably > not too bad to work around just by using a different FILE that can > vary, but if things like perror also get used it could be a lot more > of a pain. That's exactly the thing. So far I'm not passing a FILE pointer down a gazillion code paths which can possibly emit error messages. They also do not pass FILEs down into "print error message [and die]" utility functions. >> > I don't see how this is a problem unless you can read scripts from a >> > non-seekable stream, which sounds really dubious. >> >> I just tried: "bash /dev/tty" and "echo uname | bash /dev/fd/0" >> works. I prefer to not needlessly prevent my shell from being able >> to do that too. > > Is this just a theoretical concern or something you've encountered in > the wild? So far it's theoretical. Just aiming to be no worse than "competition" :) > FWIW I noticed a while back (and got bit badly) by hush's > failure to safely remap the fd it's reading the script from, so I > think replacing "fails badly as soon as the script exceeds stdio > buffer size" with "just fails when using non-seekable script input" > would be a significant improvement already. Those are/were bugs and I'm fixing them as I find them. Fixed a bunch of more obscure ones recently. IOW: I replace the situation with "works correctly regardless of script size and and redirections" situation. And currently I have one more bug in my sight: . ./SCRIPT # opened to fd 10 # in SCRIPT: exec 10>FILE To fix this, I will have to ditch usage of FILE for script reading. Or find a way to explain to FILE that its fd has been moved. >> One easy solution is to ditch FILE and use my own buffering. >> ash/dash already did exactly that many years ago, for this very reason. >> >> This would mean that libc has some 25 kb of really good, well-debugged >> I/O buffering code which I am not using because it misses >> just one (!) trivial operation in the API, fp->fd = new_fd. > > Poking through an abstraction to change internal state in a way that > nobody has specified or thought out is trivial to implement, but not > necessarily trivial to deal with the consequences of. > > Let me give a subtle example from somewhere different: > malloc_usable_size(): > > You'd expect this function to be pretty simple and non-costly to > support. The implementation has to know how much memory it allocated > for a block in order to free it, so you'd think malloc_usable_size > would not impose any implementation burden to store additional > information about the allocation. Unfortunately, that's not correct. > Suppose you call malloc(19) and, due to alignment issues, the > implementation actually has to have the allocation end at 24 bytes > rather than just 19. Now, if malloc_usable_size returns 24, the caller > can assume it got lucky and can use indices 19-23 before having to > realloc to get more, right? Nope. > > Unfortunately, the actual allocation size of 19 may be visible to the > compiler (_FORTIFY_SOURCE/__builtin_object_size, and/or > UBSan/ASan/etc.) at the point indices 19-23 are accessed, resulting in > a trap for the (UB) accesses after malloc_usable_size told the caller > it was okay to access them! So now we're stuck with a nonstandard > hackish function we should never have implemented, malloc_usable_size, > and need to find a place for the implementation to store the actual > nominal size (19) in addition to the aligned size (24) just so that > malloc_usable_size can return the right number to the caller to keep > it from invoking UB. This will either require wasted space (extra > field) or wasted time in standard codepaths (storing a non-aligned > size and rounding it up every time malloc needs to access it > internally). Bleh. Or require people to "#ifndef _FORTIFY_SOURCE" around their use of malloc_usable_size(). IOW: if they build a debug version of their code, it's their problem when some iffy optimizations cause _them_ pain. It's not something particularly new. IIRC some optimized strlen() code had the problem of sometimes reading past the string end, but never past the page, and never depending on that data, so it was fine - but did cause instrumentation to flag it as "use of uninitialized data". > I don't necessarily see something like this happening with set_fileno, > but the whole point is that we didn't see it happening with > malloc_usable_size either, until it did. > >> This was the thought which prompted me to write the email: >> by seeing how much this would help, you might agree. > > I do see how helpful it would be, but there are also very good reasons > I'm cautious (and the musl community tends to be very skeptical) of > this sort of thing, which I hope you can understand too. I understand that pushback is a good thing: you need me to show you why adding new $FEATURE is bringing more benefits that costs. I propose to reduce costs for you by allowing set_fd(fp, fd) to fail. Also, it should be explicitly defined that any buffered input or output data is not discarded or sent to previous fd during/after the call - tt only changes internal fd, if possible No flushing, no seeking. Essentially, user has to take care and fflush(fp) before he starts messing with fds, or buffered data may end up up written to a wrong fd. > Do you know what portion of the ~25k of buffered IO code (stdio) from > musl you're actually using for parsing scripts? If it's mostly just > getc and ungetc Most of the printf formatting machinery is used because of messages. IOW: I suspect large part of stdio is used. It's a shame to link in all that code and _then_ also add a reimplementation of part of it. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-02 16:02 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-31 15:05 shell needs to change fd in a FILE Denys Vlasenko 2017-07-31 20:18 ` Rich Felker 2017-08-02 12:08 ` Denys Vlasenko 2017-08-02 15:38 ` Rich Felker 2017-08-02 16:02 ` Denys Vlasenko
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).