Re: [OSS-Tools] [PATCH 2/8] libdt: factor out u64 sysattr parsing into helper
On Fri, Jun 02, 2023 at 03:16:13PM +0200, Roland Hieber wrote:
> On Wed, May 31, 2023 at 05:22:47PM +0200, Ahmad Fatoum wrote:
> > We will need to parse two more sysattrs in a follow-up patch, so factor
> > this out for reuse. While at it switch to 64-bit strtoull: This may
> > be more useful for future users and unlike atol doesn't invoke undefined
> > behavior when parsing fails.
> >
> > Signed-off-by: Ahmad Fatoum
> > ---
> > src/libdt.c | 42 +++---
> > 1 file changed, 35 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libdt.c b/src/libdt.c
> > index 2c994c647ac9..580b0b0ba769 100644
> > --- a/src/libdt.c
> > +++ b/src/libdt.c
> > @@ -2179,6 +2179,33 @@ static struct udev_device
> > *device_find_mtd_partition(struct udev_device *dev,
> > return NULL;
> > }
> >
> > +/*
>
> Use /** here so API doc generators could potentially pick it up (even
> if we don't build API docs right now, but all other functions are
> documented the same way).
OK disregard that, not all functions have /** comments */ above them, so
this is a separate issue.
- Roland
> > + * udev_device_parse_sysattr_u64 - parse sysattr value as unsigned 64-bit
> > integer
> > + * @dev: the udev_device to extract the attribute from
> > + * @attr: name of the attribute to lookup
> > + * @outvalue: returns the value parsed out of the attribute
> > + *
> > + * returns 0 for success or negative error value on failure.
> > + */
> > +static int udev_device_parse_sysattr_u64(struct udev_device *dev, const
> > char *attr,
> > + u64 *outvalue)
> > +{
> > + char *endptr;
> > + u64 value;
> > + const char *str;
> > +
> > + str = udev_device_get_sysattr_value(dev, attr);
> > + if (!str)
> > + return -EINVAL;
> > +
> > + value = strtoull(str, , 0);
> > + if (!*str || *endptr)
> > + return -EINVAL;
> > +
> > + *outvalue = value;
> > + return 0;
> > +}
> > +
> > /*
> > * device_find_block_device - extract device path from udev block device
> > *
> > @@ -2305,24 +2332,25 @@ static int udev_device_is_eeprom(struct udev_device
> > *dev)
> > * udev_parse_mtd - get information from a mtd udev_device
> > * @dev: the udev_device to extract information from
> > * @devpath: returns the devicepath under which the mtd device is
> > accessible
> > - * @size: returns the size of the mtd device
> > + * @outsize: returns the size of the mtd device
> > *
> > * returns 0 for success or negative error value on failure. *devpath
> > * will be valid on success and must be freed after usage.
> > */
> > -static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t
> > *size)
> > +static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t
> > *outsize)
> > {
> > - const char *sizestr;
> > const char *outpath;
> > + u64 size;
> > + int ret;
> >
> > if (!udev_device_is_mtd(dev))
> > return -EINVAL;
> >
> > - sizestr = udev_device_get_sysattr_value(dev, "size");
> > - if (!sizestr)
> > - return -EINVAL;
> > + ret = udev_device_parse_sysattr_u64(dev, "size", );
> > + if (ret)
> > + return ret;
> >
> > - *size = atol(sizestr);
> > + *outsize = size;
> >
> > outpath = udev_device_get_devnode(dev);
> > if (!outpath)
> > --
> > 2.39.2
> >
> >
> >
>
> --
> Roland Hieber, Pengutronix e.K. | r.hie...@pengutronix.de |
> Steuerwalder Str. 21 | https://www.pengutronix.de/ |
> 31137 Hildesheim, Germany| Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
>
>
--
Roland Hieber, Pengutronix e.K. | r.hie...@pengutronix.de |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany| Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [OSS-Tools] [PATCH 2/8] libdt: factor out u64 sysattr parsing into helper
On Wed, May 31, 2023 at 05:22:47PM +0200, Ahmad Fatoum wrote:
> We will need to parse two more sysattrs in a follow-up patch, so factor
> this out for reuse. While at it switch to 64-bit strtoull: This may
> be more useful for future users and unlike atol doesn't invoke undefined
> behavior when parsing fails.
>
> Signed-off-by: Ahmad Fatoum
> ---
> src/libdt.c | 42 +++---
> 1 file changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/src/libdt.c b/src/libdt.c
> index 2c994c647ac9..580b0b0ba769 100644
> --- a/src/libdt.c
> +++ b/src/libdt.c
> @@ -2179,6 +2179,33 @@ static struct udev_device
> *device_find_mtd_partition(struct udev_device *dev,
> return NULL;
> }
>
> +/*
Use /** here so API doc generators could potentially pick it up (even
if we don't build API docs right now, but all other functions are
documented the same way).
- Roland
> + * udev_device_parse_sysattr_u64 - parse sysattr value as unsigned 64-bit
> integer
> + * @dev: the udev_device to extract the attribute from
> + * @attr:name of the attribute to lookup
> + * @outvalue:returns the value parsed out of the attribute
> + *
> + * returns 0 for success or negative error value on failure.
> + */
> +static int udev_device_parse_sysattr_u64(struct udev_device *dev, const char
> *attr,
> + u64 *outvalue)
> +{
> + char *endptr;
> + u64 value;
> + const char *str;
> +
> + str = udev_device_get_sysattr_value(dev, attr);
> + if (!str)
> + return -EINVAL;
> +
> + value = strtoull(str, , 0);
> + if (!*str || *endptr)
> + return -EINVAL;
> +
> + *outvalue = value;
> + return 0;
> +}
> +
> /*
> * device_find_block_device - extract device path from udev block device
> *
> @@ -2305,24 +2332,25 @@ static int udev_device_is_eeprom(struct udev_device
> *dev)
> * udev_parse_mtd - get information from a mtd udev_device
> * @dev: the udev_device to extract information from
> * @devpath: returns the devicepath under which the mtd device is accessible
> - * @size:returns the size of the mtd device
> + * @outsize: returns the size of the mtd device
> *
> * returns 0 for success or negative error value on failure. *devpath
> * will be valid on success and must be freed after usage.
> */
> -static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t
> *size)
> +static int udev_parse_mtd(struct udev_device *dev, char **devpath, size_t
> *outsize)
> {
> - const char *sizestr;
> const char *outpath;
> + u64 size;
> + int ret;
>
> if (!udev_device_is_mtd(dev))
> return -EINVAL;
>
> - sizestr = udev_device_get_sysattr_value(dev, "size");
> - if (!sizestr)
> - return -EINVAL;
> + ret = udev_device_parse_sysattr_u64(dev, "size", );
> + if (ret)
> + return ret;
>
> - *size = atol(sizestr);
> + *outsize = size;
>
> outpath = udev_device_get_devnode(dev);
> if (!outpath)
> --
> 2.39.2
>
>
>
--
Roland Hieber, Pengutronix e.K. | r.hie...@pengutronix.de |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany| Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [OSS-Tools] [PATCH 5/5] barebox-state: fix use after free in error path
For the whole series:
Reviewed-by: Roland Hieber
On Wed, May 31, 2023 at 05:10:15PM +0200, Ahmad Fatoum wrote:
> blob_bin is freed a few lines above unconditionally, so freeing it
> again in the error path will cause a double free.
>
> Signed-off-by: Ahmad Fatoum
> ---
> src/keystore-blob.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/keystore-blob.c b/src/keystore-blob.c
> index ed6ecb4eaa25..8ec07f0a3d56 100644
> --- a/src/keystore-blob.c
> +++ b/src/keystore-blob.c
> @@ -81,10 +81,8 @@ int keystore_get_secret(const char *name, const unsigned
> char **key, int *key_le
>
> /* payload */
> fd = open(blob_gen_payload, O_RDONLY);
> - if (fd < 0) {
> - free(blob_bin);
> + if (fd < 0)
> return -errno;
> - }
>
> payload = xzalloc(len);
> len = read(fd, payload, len);
> --
> 2.39.2
>
>
>
--
Roland Hieber, Pengutronix e.K. | r.hie...@pengutronix.de |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany| Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
