Hi Mehdi,
Thanks for the review.
On Thu, Dec 05, 2024 at 10:54:10AM +0100, Mehdi Djait wrote:
> Hi Sakari,
>
> On Thu, Dec 05, 2024 at 11:44:46AM +0200, Sakari Ailus wrote:
> > ccs_data_parse() releases the allocated in-memory data structure when the
> > parser fails, but it does not clean up parsed metadata that is there to
> > help access the actual data. Do that, in order to return the data
> > structure in a sane state.
> >
> > Reported-by: David Heidelberg <david@xxxxxxx>
> > Fixes: a6b396f410b1 ("media: ccs: Add CCS static data parser library")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > since v2:
> >
> > - Properly clean up after all error cases.
> >
> > drivers/media/i2c/ccs/ccs-data.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ccs/ccs-data.c b/drivers/media/i2c/ccs/ccs-data.c
> > index 9d42137f4799..2591dba51e17 100644
> > --- a/drivers/media/i2c/ccs/ccs-data.c
> > +++ b/drivers/media/i2c/ccs/ccs-data.c
> > @@ -10,6 +10,7 @@
> > #include <linux/limits.h>
> > #include <linux/mm.h>
> > #include <linux/slab.h>
> > +#include <linux/string.h>
> >
> > #include "ccs-data-defs.h"
> >
> > @@ -948,15 +949,15 @@ int ccs_data_parse(struct ccs_data_container *ccsdata, const void *data,
> >
> > rval = __ccs_data_parse(&bin, ccsdata, data, len, dev, verbose);
> > if (rval)
> > - return rval;
> > + goto out_cleanup;
> >
> > rval = bin_backing_alloc(&bin);
> > if (rval)
> > - return rval;
> > + goto out_cleanup;
> >
> > rval = __ccs_data_parse(&bin, ccsdata, data, len, dev, false);
> > if (rval)
> > - goto out_free;
> > + goto out_cleanup;
> >
> > if (verbose && ccsdata->version)
> > print_ccs_data_version(dev, ccsdata->version);
> > @@ -965,15 +966,16 @@ int ccs_data_parse(struct ccs_data_container *ccsdata, const void *data,
> > rval = -EPROTO;
> > dev_dbg(dev, "parsing mismatch; base %p; now %p; end %p\n",
> > bin.base, bin.now, bin.end);
> > - goto out_free;
> > + goto out_cleanup;
> > }
> >
> > ccsdata->backing = bin.base;
> >
> > return 0;
> >
> > -out_free:
> > +out_cleanup:
>
> Don't you think some kind of logging or at least a dev_dbg() would be
> helpful here to let the user know that ccs_data_parse() failed ?
Good question. I think it's a good idea to print a warning, probably on
warning level. I alsoI think it should be a separate patch.
Could you write one? :-)
>
> > kvfree(bin.base);
> > + memset(ccsdata, 0, sizeof(*ccsdata));
> >
> > return rval;
> > }
--
Kind regards,
Sakari Ailus
