[i-g-t,1/3] Avoid truncate string in __igt_lsof_fds
diff mbox

Message ID 03a936e0c4bca03f89e731d2cacaa3624a10147f.1527639529.git.rodrigosiqueiramelo@gmail.com
State New
Headers show

Commit Message

Rodrigo Siqueira May 30, 2018, 12:46 a.m. UTC
Note that 'proc_path' parameter in __igt_lsof_fds receives a string
which was initialized with the size of PATH_MAX and the local variable
'path' has the same size, but it also have to append: '/', '\0', and the
directory name. This situation caused the warning described below.

warning: ‘%s’ directive output may be truncated writing up to 255 bytes
into a region of size between 0 and 4095 [-Wformat-truncation=]
snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
note: ‘snprintf’ output between 2 and 4352 bytes into a destination of
size 4096 [..]

This patch fix the above problem.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 lib/igt_aux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Arkadiusz Hiler June 4, 2018, 10:40 a.m. UTC | #1
On Tue, May 29, 2018 at 09:46:38PM -0300, Rodrigo Siqueira wrote:
> Note that 'proc_path' parameter in __igt_lsof_fds receives a string
> which was initialized with the size of PATH_MAX and the local variable
> 'path' has the same size, but it also have to append: '/', '\0', and the
> directory name. This situation caused the warning described below.
> 
> warning: ‘%s’ directive output may be truncated writing up to 255 bytes
> into a region of size between 0 and 4095 [-Wformat-truncation=]
> snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
> note: ‘snprintf’ output between 2 and 4352 bytes into a destination of
> size 4096 [..]
> 
> This patch fix the above problem.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  lib/igt_aux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index acafb713..5dd2761e 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -1425,7 +1425,7 @@ __igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
>  {
>  	struct dirent *d;
>  	struct stat st;
> -	char path[PATH_MAX];
> +	char path[PATH_MAX + NAME_MAX + 2]; // 1 byte for '/' and another for '\0'

Hey,

First of, thanks for looking at the new warnings. We definitely have to
fix those :-)

A couple of lines down we have lstat(path, &st), which will return -1
with errno == ENAMETOOLONG if we go over PATH_MAX, so this does not feel
right, especially that we are not checking for that.

Digging deeper, __igt_lsof_fds() is used only inside __igt_lsof(), which
is the reason for such a high estimate.

It uses char path[PATH_MAX], which is the main contributing component,
but it contains at most strlen("/proc/%d/cwd")+1 where "%d" is
CEILING(LOG_10(INT_MAX)).

Limiting size of path there to sensible upper estimation should make us
fit in PATH_MAX in __igt_lsof_fds (or even less).
Rodrigo Siqueira June 5, 2018, 1:45 a.m. UTC | #2
Hi Arkadiusz,

First of all, thanks for the review.

I believe that I understood all the comments you made in all of the
patches. I will send the V2 soon.

Thanks!

Best Regards
Rodrigo Siqueira

On 06/04, Arkadiusz Hiler wrote:
> On Tue, May 29, 2018 at 09:46:38PM -0300, Rodrigo Siqueira wrote:
> > Note that 'proc_path' parameter in __igt_lsof_fds receives a string
> > which was initialized with the size of PATH_MAX and the local variable
> > 'path' has the same size, but it also have to append: '/', '\0', and the
> > directory name. This situation caused the warning described below.
> > 
> > warning: ‘%s’ directive output may be truncated writing up to 255 bytes
> > into a region of size between 0 and 4095 [-Wformat-truncation=]
> > snprintf(path, sizeof(path), "%s/%s", proc_path, d->d_name);
> > note: ‘snprintf’ output between 2 and 4352 bytes into a destination of
> > size 4096 [..]
> > 
> > This patch fix the above problem.
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  lib/igt_aux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index acafb713..5dd2761e 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -1425,7 +1425,7 @@ __igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
> >  {
> >  	struct dirent *d;
> >  	struct stat st;
> > -	char path[PATH_MAX];
> > +	char path[PATH_MAX + NAME_MAX + 2]; // 1 byte for '/' and another for '\0'
> 
> Hey,
> 
> First of, thanks for looking at the new warnings. We definitely have to
> fix those :-)
> 
> A couple of lines down we have lstat(path, &st), which will return -1
> with errno == ENAMETOOLONG if we go over PATH_MAX, so this does not feel
> right, especially that we are not checking for that.
> 
> Digging deeper, __igt_lsof_fds() is used only inside __igt_lsof(), which
> is the reason for such a high estimate.
> 
> It uses char path[PATH_MAX], which is the main contributing component,
> but it contains at most strlen("/proc/%d/cwd")+1 where "%d" is
> CEILING(LOG_10(INT_MAX)).
> 
> Limiting size of path there to sensible upper estimation should make us
> fit in PATH_MAX in __igt_lsof_fds (or even less).
> 
> -- 
> Cheers,
> Arek

Patch
diff mbox

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index acafb713..5dd2761e 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -1425,7 +1425,7 @@  __igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir)
 {
 	struct dirent *d;
 	struct stat st;
-	char path[PATH_MAX];
+	char path[PATH_MAX + NAME_MAX + 2]; // 1 byte for '/' and another for '\0'
 	char *fd_lnk;
 
 	/* default fds or kernel threads */