Possible oversight in setvbuf()

Possible oversight in setvbuf()

[Markus Wichmann]

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

Re: Possible oversight in setvbuf()

[Rich Felker]

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

Re: Possible oversight in setvbuf()

[Christopher Friedt]

[-- 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 --]

Re: Possible oversight in setvbuf()

[Rich Felker]

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

Re: Possible oversight in setvbuf()

[Christopher Friedt]

[-- 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 --]

Re: Possible oversight in setvbuf()

[Rich Felker]

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

Re: Possible oversight in setvbuf()

[Rich Felker]

[-- 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;
}