[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