* Possible oversight in setvbuf() @ 2018-07-13 14:40 Markus Wichmann 2018-07-13 21:31 ` Rich Felker 2018-07-14 2:05 ` Rich Felker 0 siblings, 2 replies; 7+ messages in thread From: Markus Wichmann @ 2018-07-13 14:40 UTC (permalink / raw) To: musl Hi all, ungetc() seems to depend on f->buf pointing UNGET bytes into a valid array. fdopen() will provide such a thing. However, setvbuf() will set f->buf to the very start of the user provided buffer. Bizarrely, UNGET is deducted from the buffer size, but not added to the pointer. Oversight or intentional? Ciao, Markus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 14:40 Possible oversight in setvbuf() Markus Wichmann @ 2018-07-13 21:31 ` Rich Felker 2018-07-13 21:46 ` Christopher Friedt 2018-07-14 2:05 ` Rich Felker 1 sibling, 1 reply; 7+ messages in thread From: Rich Felker @ 2018-07-13 21:31 UTC (permalink / raw) To: musl On Fri, Jul 13, 2018 at 04:40:52PM +0200, Markus Wichmann wrote: > Hi all, > > ungetc() seems to depend on f->buf pointing UNGET bytes into a valid > array. fdopen() will provide such a thing. However, setvbuf() will set > f->buf to the very start of the user provided buffer. Bizarrely, UNGET > is deducted from the buffer size, but not added to the pointer. > Oversight or intentional? Definitely a bug -- thanks for catching this. I guess it's a good thing that the release has been delayed for a while, so it won't be in the wild except where musl git master is in use rather than a release. One thing this highlights is that we could really use better testing and security review process. I'll write and submit a test to libc-test for this, but I'd really like if it we could find someone using musl willing to sponsor continuous or periodic security reviews of changes by a third party. Rich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 21:31 ` Rich Felker @ 2018-07-13 21:46 ` Christopher Friedt 2018-07-13 22:06 ` Rich Felker 0 siblings, 1 reply; 7+ messages in thread From: Christopher Friedt @ 2018-07-13 21:46 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 447 bytes --] On Fri, Jul 13, 2018, 5:31 PM Rich Felker, <dalias@libc.org> wrote: > One thing this highlights is that we could really use better testing > and security review process. I'll write and > Using some static analysis tool would be good too - not sure if that's in use already. A good process for adding new features is to add tests with them. Even if there is only a test for expected behaviour, at least it will catch one possible regression. > [-- Attachment #2: Type: text/html, Size: 971 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 21:46 ` Christopher Friedt @ 2018-07-13 22:06 ` Rich Felker 2018-07-13 22:19 ` Christopher Friedt 0 siblings, 1 reply; 7+ messages in thread From: Rich Felker @ 2018-07-13 22:06 UTC (permalink / raw) To: musl On Fri, Jul 13, 2018 at 05:46:54PM -0400, Christopher Friedt wrote: > On Fri, Jul 13, 2018, 5:31 PM Rich Felker, <dalias@libc.org> wrote: > > > One thing this highlights is that we could really use better testing > > and security review process. I'll write and > > > > Using some static analysis tool would be good too - not sure if that's in > use already. We have in the past, and they caught a small number of real issues along with a lot of false positives. Stuff like this is hard for static analysis to test without also having knowledge of the relevant interface contract(s). > A good process for adding new features is to add tests with them. Even if > there is only a test for expected behaviour, at least it will catch one > possible regression. Yes, I should really do that more. Sometimes it's not obvious what should be tested though. In the case of setvbuf, the intended behavior is in some sense untestable (the previous implementation not using the caller-provided buffer was valid); in hindsight the obvious important thing to test is that it doesn't result in writes outside the buffer. Rich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 22:06 ` Rich Felker @ 2018-07-13 22:19 ` Christopher Friedt 2018-07-14 2:03 ` Rich Felker 0 siblings, 1 reply; 7+ messages in thread From: Christopher Friedt @ 2018-07-13 22:19 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 814 bytes --] On Fri, Jul 13, 2018, 6:06 PM Rich Felker, <dalias@libc.org> wrote: > On Fri, Jul 13, 2018 at 05:46:54PM -0400, Christopher Friedt wrote: > > A good process for adding new features is to add tests with them. Even if > > there is only a test for expected behaviour, at least it will catch one > > possible regression. > > Yes, I should really do that more. Sometimes it's not obvious what > should be tested though. In the case of setvbuf, the intended behavior > is in some sense untestable (the previous implementation not using the > caller-provided buffer was valid); in hindsight the obvious important > thing to test is that it doesn't result in writes outside the buffer. > If you need more than that basic test.c program that I provided before for addrconfig, i'd be happy to add some more test cases. > [-- Attachment #2: Type: text/html, Size: 1330 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 22:19 ` Christopher Friedt @ 2018-07-14 2:03 ` Rich Felker 0 siblings, 0 replies; 7+ messages in thread From: Rich Felker @ 2018-07-14 2:03 UTC (permalink / raw) To: musl On Fri, Jul 13, 2018 at 06:19:30PM -0400, Christopher Friedt wrote: > On Fri, Jul 13, 2018, 6:06 PM Rich Felker, <dalias@libc.org> wrote: > > > On Fri, Jul 13, 2018 at 05:46:54PM -0400, Christopher Friedt wrote: > > > A good process for adding new features is to add tests with them. Even if > > > there is only a test for expected behaviour, at least it will catch one > > > possible regression. > > > > Yes, I should really do that more. Sometimes it's not obvious what > > should be tested though. In the case of setvbuf, the intended behavior > > is in some sense untestable (the previous implementation not using the > > caller-provided buffer was valid); in hindsight the obvious important > > thing to test is that it doesn't result in writes outside the buffer. > > > > If you need more than that basic test.c program that I provided before for > addrconfig, i'd be happy to add some more test cases. The hard thing about testing that functionality is that it's dependent on environmental factors (network interface configuration). The same problem also applies to the rest of the resolver functionality, where you'd want to be able to hook it up to a known, and possibly malicious, nameserver that's part of the test harness. There are two "obvious" choices for how to achieve this: 1. Using Linux namespaces (and assuming user namespace functionality is available to access them, which requires either appropriate configuration or root) so that control over the filesystem and network interfaces is available. Or, 2. Implementing a tracer (perhaps just seccomp) process that catches and rewrites the syscall results to simulate the conditions we want to test. Neither is particularly portable outside of Linux (note that libc-test is OS- and libc-agnostic). Option 1 is cleaner and simpler and more flexible, but won't work on all systems (and especially not on more hardened ones). I'm not sure if Szabolcs Nagy (libc-test maintainer) likes either idea. Rich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Possible oversight in setvbuf() 2018-07-13 14:40 Possible oversight in setvbuf() Markus Wichmann 2018-07-13 21:31 ` Rich Felker @ 2018-07-14 2:05 ` Rich Felker 1 sibling, 0 replies; 7+ messages in thread From: Rich Felker @ 2018-07-14 2:05 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 482 bytes --] On Fri, Jul 13, 2018 at 04:40:52PM +0200, Markus Wichmann wrote: > Hi all, > > ungetc() seems to depend on f->buf pointing UNGET bytes into a valid > array. fdopen() will provide such a thing. However, setvbuf() will set > f->buf to the very start of the user provided buffer. Bizarrely, UNGET > is deducted from the buffer size, but not added to the pointer. > Oversight or intentional? I'm committing the obvious fix. Attached is a regression test suitable for libc-test. Rich [-- Attachment #2: setvbuf-unget.c --] [-- Type: text/plain, Size: 422 bytes --] // commit: 9cad27a3dc1a4eb349b6591e4dc8cc89dce32277 // ungetc after setvbuf should not clobber memory below buffer #include <stdio.h> #include <string.h> #include "test.h" int main(void) { char buf[1024] = "hello world"; setvbuf(stdin, buf+12, _IOFBF, sizeof buf - 12); while (ungetc('x', stdin)!=EOF); if (strcmp(buf, "hello world")) t_error("ungetc clobbered outside buffer: [%.12s]\n", buf); return t_status; } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-07-14 2:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-13 14:40 Possible oversight in setvbuf() Markus Wichmann 2018-07-13 21:31 ` Rich Felker 2018-07-13 21:46 ` Christopher Friedt 2018-07-13 22:06 ` Rich Felker 2018-07-13 22:19 ` Christopher Friedt 2018-07-14 2:03 ` Rich Felker 2018-07-14 2:05 ` Rich Felker
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).