[igt-dev] [PATCH i-g-t v4 4/9] lib/igt_kmod: don't leak who from module unload routines

Mauro Carvalho Chehab mauro.chehab at linux.intel.com
Tue May 17 11:04:24 UTC 2022


On Tue, 17 May 2022 12:24:08 +0200
Andi Shyti <andi.shyti at linux.intel.com> wrote:

> Hi Mauro,
> 
> On Tue, May 17, 2022 at 09:27:58AM +0200, Mauro Carvalho Chehab wrote:
> > From: Mauro Carvalho Chehab <mchehab at kernel.org>
> > 
> > Add code to free allocated strings at the module unload routines
> > from igt_kmod.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab at kernel.org>
> > ---
> >  lib/igt_kmod.c        | 36 +++++++++++++++++++++++++-----------
> >  lib/igt_kmod.h        |  4 ++--
> >  tests/i915/perf_pmu.c |  4 +++-
> >  3 files changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index ca74c602ce66..3e9302686844 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -390,7 +390,7 @@ igt_i915_driver_load(const char *opts)
> >  	return 0;
> >  }
> >  
> > -static int igt_always_unload_audio_driver(const char **who)
> > +static int igt_always_unload_audio_driver(char **who)
> >  {
> >  	int ret;
> >  	const char *sound[] = {
> > @@ -401,8 +401,11 @@ static int igt_always_unload_audio_driver(const char **who)
> >  
> >  	for (const char **m = sound; *m; m++) {
> >  		if (igt_kmod_is_loaded(*m)) {
> > -			if (who)
> > -				*who = *m;
> > +			if (who) {
> > +				if (*who)
> > +					free(*who);  
> 
> I think free() is NULL safe. At least that's what the
> documentation says(*).

Maybe on some versions of glibc, but I'm pretty sure I got already
troubles with free(NULL).

> We could eventually make an igt_strdup_after_free(ptr) function
> similar to this:
> 
>   char *igt_strdup_after_free(char **ptr, char *m)
>   {
>   	free(*ptr); /* don't know if it's required a double ** */
>   	return strdup(m);
>   }
> 
> and place it before patch 3. Then we just replace
> /strdup/igt_strdup_after_free/ .

I would prefer to define it as something like:

static void *strealloc(char *origptr, char *strdata)
{
    size_t nbytes = strlen(strdata) + 1;
    char *newptr = realloc(origptr, nbytes);
    memcpy(newptr, strdata, nbytes);
    return newptr;
}

Yet, it sounds overkill to me ;-)

> 
> This way we also have a correct bisectability.
> 
> What do you think? You can choose a better name, I'm not good at
> naming functions.
> 
> Andi
> 
> (*) If ptr is NULL, no operation is performed.
> 
> > +				*who = strdup(*m);
> > +			}
> >  			if (igt_lsof_kill_audio_processes())
> >  				return EACCES;
> >  
> > @@ -544,7 +547,7 @@ static int linux_kernel_version(void)
> >  	return LINUX_VERSION(ver[0], ver[1], ver[2]);
> >  }
> >  
> > -int igt_audio_driver_unload(const char **who)
> > +int igt_audio_driver_unload(char **who)
> >  {
> >  	const char *drm_driver = "i915";
> >  	unsigned int num_mod, i, j;
> > @@ -583,8 +586,11 @@ int igt_audio_driver_unload(const char **who)
> >  	/* Recursively remove all drivers that depend on drm_driver */
> >  	for (j = 0; j < mod[i].num_required; j++) {
> >  		pos = mod[i].required_by[j];
> > -		if (who)
> > +		if (who) {
> > +			if (*who)
> > +				free(who);
> >  			*who = strdup(mod[pos].name);
> > +		}
> >  		/*
> >  		 * If a sound driver depends on drm_driver, kill audio processes
> >  		 * first, in order to make it possible to unload the driver
> > @@ -604,7 +610,7 @@ ret:
> >  	return ret;
> >  }
> >  
> > -int __igt_i915_driver_unload(const char **who)
> > +int __igt_i915_driver_unload(char **who)
> >  {
> >  	int ret;
> >  
> > @@ -631,8 +637,12 @@ int __igt_i915_driver_unload(const char **who)
> >  
> >  		ret = igt_kmod_unload(*m, 0);
> >  		if (ret) {
> > -			if (who)
> > -				*who = *m;
> > +			if (who) {
> > +				if (*who)
> > +					free(*who);
> > +
> > +				*who = strdup(*m);
> > +			}
> >  			return ret;
> >  		}
> >  	}
> > @@ -640,8 +650,11 @@ int __igt_i915_driver_unload(const char **who)
> >  	if (igt_kmod_is_loaded("i915")) {
> >  		ret = igt_kmod_unload("i915", 0);
> >  		if (ret) {
> > -			if (who)
> > -				*who = "i915";
> > +			if (who) {
> > +				if (*who)
> > +					free(*who);
> > +				*who = strdup("i915");
> > +			}
> >  			return ret;
> >  		}
> >  	}
> > @@ -658,7 +671,7 @@ int __igt_i915_driver_unload(const char **who)
> >  int
> >  igt_i915_driver_unload(void)
> >  {
> > -	const char *who;
> > +	char *who = NULL;
> >  	int ret;
> >  
> >  	ret = __igt_i915_driver_unload(&who);
> > @@ -667,6 +680,7 @@ igt_i915_driver_unload(void)
> >  		igt_kmod_list_loaded();
> >  		igt_lsof("/dev/dri");
> >  		igt_lsof("/dev/snd");
> > +		free(who);
> >  		return ret;
> >  	}
> >  
> > diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > index 15f0be31e8e4..f98dd29fb175 100644
> > --- a/lib/igt_kmod.h
> > +++ b/lib/igt_kmod.h
> > @@ -36,11 +36,11 @@ bool igt_kmod_has_param(const char *mod_name, const char *param);
> >  int igt_kmod_load(const char *mod_name, const char *opts);
> >  int igt_kmod_unload(const char *mod_name, unsigned int flags);
> >  
> > -int igt_audio_driver_unload(const char **whom);
> > +int igt_audio_driver_unload(char **whom);
> >  
> >  int igt_i915_driver_load(const char *opts);
> >  int igt_i915_driver_unload(void);
> > -int __igt_i915_driver_unload(const char **whom);
> > +int __igt_i915_driver_unload(char **whom);
> >  
> >  int igt_amdgpu_driver_load(const char *opts);
> >  int igt_amdgpu_driver_unload(void);
> > diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> > index c48cc07df0d2..ce047eab8296 100644
> > --- a/tests/i915/perf_pmu.c
> > +++ b/tests/i915/perf_pmu.c
> > @@ -2047,7 +2047,7 @@ static void test_unload(unsigned int num_engines)
> >  		int fd[4 + num_engines * 3], i;
> >  		uint64_t *buf;
> >  		int count = 0, ret;
> > -		const char *who;
> > +		char *who = NULL;
> >  		int i915;
> >  
> >  		i915 = __drm_open_driver(DRIVER_INTEL);
> > @@ -2104,6 +2104,8 @@ static void test_unload(unsigned int num_engines)
> >  		pmu_read_multi(fd[0], count, buf);
> >  		ret = __igt_i915_driver_unload(&who);
> >  		igt_assert(ret != 0 && !strcmp(who, "i915"));
> > +		if (who)
> > +			free(who);
> >  		pmu_read_multi(fd[0], count, buf);
> >  
> >  		igt_debug("Close perf\n");
> > -- 
> > 2.36.1  


More information about the igt-dev mailing list