[iproute,51/51] lib/bpf: Check return value of write()

Submitted by Phil Sutter on Aug. 12, 2017, 12:05 p.m.

Details

Message ID 20170812120510.28750-52-phil@nwl.cc
State Changes Requested
Delegated to: stephen hemminger
Headers show

Commit Message

Phil Sutter Aug. 12, 2017, 12:05 p.m.
This is merely to silence the compiler warning. If write to stderr
failed, assume that printing an error message will fail as well so don't
even try.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 lib/bpf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Borkmann Aug. 14, 2017, 9:17 a.m.
On 08/12/2017 02:05 PM, Phil Sutter wrote:
> This is merely to silence the compiler warning. If write to stderr
> failed, assume that printing an error message will fail as well so don't
> even try.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>   lib/bpf.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 1dcb261dc915f..825e071cea572 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
>
>   		ret = read(fd, buff, sizeof(buff) - 1);
>   		if (ret > 0) {
> -			write(2, buff, ret);
> +			if (write(STDERR_FILENO, buff, ret) != ret)
> +				return -1;

Quite unlikely to fail, but we should probably bark loudly
here instead of just returning -1. Perhaps assert() would
suit better.

>   			fflush(stderr);
>   		}
>   	}
>
Phil Sutter Aug. 14, 2017, 5:25 p.m.
On Mon, Aug 14, 2017 at 11:17:39AM +0200, Daniel Borkmann wrote:
> On 08/12/2017 02:05 PM, Phil Sutter wrote:
> > This is merely to silence the compiler warning. If write to stderr
> > failed, assume that printing an error message will fail as well so don't
> > even try.
> >
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >   lib/bpf.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/bpf.c b/lib/bpf.c
> > index 1dcb261dc915f..825e071cea572 100644
> > --- a/lib/bpf.c
> > +++ b/lib/bpf.c
> > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
> >
> >   		ret = read(fd, buff, sizeof(buff) - 1);
> >   		if (ret > 0) {
> > -			write(2, buff, ret);
> > +			if (write(STDERR_FILENO, buff, ret) != ret)
> > +				return -1;
> 
> Quite unlikely to fail, but we should probably bark loudly
> here instead of just returning -1. Perhaps assert() would
> suit better.

Well, according to assert(3), it will print to stderr before aborting
the program. So if writing STDERR_FILENO failed, I guess it won't
provide much more detail to the user, either. If bpf_trace_pipe()
returns non-zero, parse_bpf() prints an error message to stderr and
returns -1. Ultimately tc will return non-zero. With stderr unfit for
writing into, I doubt there's anything left we could do besides that.

But I really think we shouldn't make such a fuss about it - writing to
stderr either always works or we're in trouble everywhere. This patch
was merely to shut gcc up, so no need to waste much energy on a scenario
which won't happen anyway.

Thanks, Phil
Daniel Borkmann Aug. 14, 2017, 8:35 p.m.
On 08/14/2017 07:25 PM, Phil Sutter wrote:
[...]
> But I really think we shouldn't make such a fuss about it - writing to
> stderr either always works or we're in trouble everywhere. This patch
> was merely to shut gcc up, so no need to waste much energy on a scenario
> which won't happen anyway.

Yup, fair enough, makes sense.

Acked-by: Daniel Borkmann <daniel@iogearbox.net>
David Laight Aug. 15, 2017, 12:31 p.m.
From: Phil Sutter
> Sent: 12 August 2017 13:05
> This is merely to silence the compiler warning. If write to stderr
> failed, assume that printing an error message will fail as well so don't
> even try.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  lib/bpf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 1dcb261dc915f..825e071cea572 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
> 
>  		ret = read(fd, buff, sizeof(buff) - 1);
>  		if (ret > 0) {
> -			write(2, buff, ret);
> +			if (write(STDERR_FILENO, buff, ret) != ret)
> +				return -1;
>  			fflush(stderr);
>  		}

WTF is this code doing anyway?
write() is a system call, fflush() writes out any data buffered in the
stdio stream.
If there was anything buffered you'd want to output it earlier.
Otherwise if it is going to use fflush() it should be using fwrite().

I presume the function is allowed to write to stderr - since in general
library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid.
There is a lot of code out there that does close(0); close(1); close(2);
but leaves stdout/err valid. Call printf() instead of sprint() and eventually
10k of data gets written somewhere rather unexpected.

If it is a copy loop, what is wrong with the last byte of buff[].
It is valid for write() to return a partial length - the code should
probably loop until all the data is accepted (or error).

	David
Daniel Borkmann Aug. 15, 2017, 1 p.m.
On 08/15/2017 02:31 PM, David Laight wrote:
[...]
> WTF is this code doing anyway?
> write() is a system call, fflush() writes out any data buffered in the
> stdio stream.
> If there was anything buffered you'd want to output it earlier.
> Otherwise if it is going to use fflush() it should be using fwrite().
>
> I presume the function is allowed to write to stderr - since in general
> library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid.
> There is a lot of code out there that does close(0); close(1); close(2);
> but leaves stdout/err valid. Call printf() instead of sprint() and eventually
> 10k of data gets written somewhere rather unexpected.
>
> If it is a copy loop, what is wrong with the last byte of buff[].
> It is valid for write() to return a partial length - the code should
> probably loop until all the data is accepted (or error).

Just send a patch if you really care; would have probably been faster
than typing up your email. ;) Thank you!

Patch hide | download patch | download mbox

diff --git a/lib/bpf.c b/lib/bpf.c
index 1dcb261dc915f..825e071cea572 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -591,7 +591,8 @@  int bpf_trace_pipe(void)
 
 		ret = read(fd, buff, sizeof(buff) - 1);
 		if (ret > 0) {
-			write(2, buff, ret);
+			if (write(STDERR_FILENO, buff, ret) != ret)
+				return -1;
 			fflush(stderr);
 		}
 	}