[PATCH iproute2] ip route: check ftell, fseek return value

[PATCH iproute2] ip route: check ftell, fseek return value

[Hangbin Liu]

ftell() may return -1 in error case, which is not handled and therefore pass a
negative offset to fseek(). The return code of fseek() is also not checked.

Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 ip/iproute.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index 3da23af..ba877dc 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1859,7 +1859,11 @@ static int iproute_restore(void)
 	if (route_dump_check_magic())
 		exit(-1);
 
-	pos = ftell(stdin);
+	if ((pos = ftell(stdin)) == -1) {
+		perror("Failed to restore: ftell");
+		exit(errno);
+	}
+
 	for (prio = 0; prio < 3; prio++) {
 		int err;
 
@@ -1867,7 +1871,10 @@ static int iproute_restore(void)
 		if (err)
 			exit(err);
 
-		fseek(stdin, pos, SEEK_SET);
+		if (fseek(stdin, pos, SEEK_SET) == -1) {
+			perror("Failed to restore: fseek");
+			exit(errno);
+		}
 	}
 
 	exit(0);
-- 
2.5.5

Re: [PATCH iproute2] ip route: check ftell, fseek return value

[Phil Sutter]

On Tue, Sep 06, 2016 at 02:39:50PM +0800, Hangbin Liu wrote:
> ftell() may return -1 in error case, which is not handled and therefore pass a
> negative offset to fseek(). The return code of fseek() is also not checked.
> 
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Acked-by: Phil Sutter <phil@nwl.cc>

RE: [PATCH iproute2] ip route: check ftell, fseek return value

[David Laight]

From: Hangbin Liu
> Sent: 06 September 2016 07:40
> ftell() may return -1 in error case, which is not handled and therefore pass a
> negative offset to fseek(). The return code of fseek() is also not checked.
> 
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  ip/iproute.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/ip/iproute.c b/ip/iproute.c
> index 3da23af..ba877dc 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -1859,7 +1859,11 @@ static int iproute_restore(void)
>  	if (route_dump_check_magic())
>  		exit(-1);
> 
> -	pos = ftell(stdin);
> +	if ((pos = ftell(stdin)) == -1) {

Don't put assignments in conditionals.

> +		perror("Failed to restore: ftell");
> +		exit(errno);

errno is not a valid argument to exit().

...

Actually WTF is this code trying to do.
stdin is very likely to be a pipe, so expecting to seek on it
seems very likely to fail.

	David

Re: [PATCH iproute2] ip route: check ftell, fseek return value

[Hangbin Liu]

On Tue, Sep 06, 2016 at 11:18:00AM +0000, David Laight wrote:
> From: Hangbin Liu
> > Sent: 06 September 2016 07:40
> > ftell() may return -1 in error case, which is not handled and therefore pass a
> > negative offset to fseek(). The return code of fseek() is also not checked.
> > 
> > Reported-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  ip/iproute.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index 3da23af..ba877dc 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -1859,7 +1859,11 @@ static int iproute_restore(void)
> >  	if (route_dump_check_magic())
> >  		exit(-1);
> > 
> > -	pos = ftell(stdin);
> > +	if ((pos = ftell(stdin)) == -1) {
> 
> Don't put assignments in conditionals.
> 
> > +		perror("Failed to restore: ftell");
> > +		exit(errno);
> 
> errno is not a valid argument to exit().

Hi David,

Thanks for the advice. I will send PATCHv2 to fix them.

> 
> ...
> 
> Actually WTF is this code trying to do.
> stdin is vere likely to be a pipe, so expecting to seek on it
> seems very likely to fail.

I will fix the return code handle issue first and try investigate the pipe
issue later.

Regards
Hangbin