[libcamera-devel,6/7] libcamera: yaml_parser: Fix range checks for 32-bit integers
diff mbox series

Message ID 20220616142403.20723-7-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: yaml_parser: Add iterator API
Related show

Commit Message

Laurent Pinchart June 16, 2022, 2:24 p.m. UTC 
The strtol() and strtoul() functions return long integers, which may be
larger than 32-bit integers. Add manual range checks.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/yaml_parser.cpp | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi June 17, 2022, 2:07 p.m. UTC | #1 
Hi Laurent

On Thu, Jun 16, 2022 at 05:24:02PM +0300, Laurent Pinchart via libcamera-devel wrote:
> The strtol() and strtoul() functions return long integers, which may be
> larger than 32-bit integers. Add manual range checks.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  src/libcamera/yaml_parser.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 9b6e70cbfcf3..bd4b501b1422 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -10,6 +10,7 @@
>  #include <cstdlib>
>  #include <errno.h>
>  #include <functional>
> +#include <limits>
>
>  #include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
> @@ -151,9 +152,11 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>  	char *end;
>
>  	errno = 0;
> -	int32_t value = std::strtol(value_.c_str(), &end, 10);

Or should value be a long instead ?

> +	long value = std::strtol(value_.c_str(), &end, 10);
>
> -	if ('\0' != *end || errno == ERANGE)
> +	if ('\0' != *end || errno == ERANGE ||
> +	    value < std::numeric_limits<int32_t>::min() ||
> +	    value > std::numeric_limits<int32_t>::max())
>  		return defaultValue;
>
>  	setOk(ok, true);
> @@ -185,9 +188,11 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>  	char *end;
>
>  	errno = 0;
> -	uint32_t value = std::strtoul(value_.c_str(), &end, 10);
> +	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
>
> -	if ('\0' != *end || errno == ERANGE)
> +	if ('\0' != *end || errno == ERANGE ||
> +	    value < std::numeric_limits<uint32_t>::min() ||
> +	    value > std::numeric_limits<uint32_t>::max())
>  		return defaultValue;
>
>  	setOk(ok, true);
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi June 17, 2022, 2:09 p.m. UTC | #2 
Ahem

On Fri, Jun 17, 2022 at 04:07:17PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Laurent
>
> On Thu, Jun 16, 2022 at 05:24:02PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The strtol() and strtoul() functions return long integers, which may be
> > larger than 32-bit integers. Add manual range checks.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> > ---
> >  src/libcamera/yaml_parser.cpp | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 9b6e70cbfcf3..bd4b501b1422 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -10,6 +10,7 @@
> >  #include <cstdlib>
> >  #include <errno.h>
> >  #include <functional>
> > +#include <limits>
> >
> >  #include <libcamera/base/file.h>
> >  #include <libcamera/base/log.h>
> > @@ -151,9 +152,11 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
> >  	char *end;
> >
> >  	errno = 0;
> > -	int32_t value = std::strtol(value_.c_str(), &end, 10);
>
> Or should value be a long instead ?
>

Wrote this before realizing this was a specialization and forgot to delete
the comment!

Thanks
  j

> > +	long value = std::strtol(value_.c_str(), &end, 10);
> >
> > -	if ('\0' != *end || errno == ERANGE)
> > +	if ('\0' != *end || errno == ERANGE ||
> > +	    value < std::numeric_limits<int32_t>::min() ||
> > +	    value > std::numeric_limits<int32_t>::max())
> >  		return defaultValue;
> >
> >  	setOk(ok, true);
> > @@ -185,9 +188,11 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
> >  	char *end;
> >
> >  	errno = 0;
> > -	uint32_t value = std::strtoul(value_.c_str(), &end, 10);
> > +	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
> >
> > -	if ('\0' != *end || errno == ERANGE)
> > +	if ('\0' != *end || errno == ERANGE ||
> > +	    value < std::numeric_limits<uint32_t>::min() ||
> > +	    value > std::numeric_limits<uint32_t>::max())
> >  		return defaultValue;
> >
> >  	setOk(ok, true);
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
Hanlin Chen June 20, 2022, 11:48 a.m. UTC | #3 
Hi Laurent

On Thu, Jun 16, 2022 at 05:24:02PM +0300, Laurent Pinchart via
libcamera-devel wrote:
> The strtol() and strtoul() functions return long integers, which may be
> larger than 32-bit integers. Add manual range checks.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Han-Lin Chen <hanlinchen@chromium.org>

Thanks for the fix!

On Thu, Jun 16, 2022 at 10:24 PM Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> The strtol() and strtoul() functions return long integers, which may be
> larger than 32-bit integers. Add manual range checks.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index 9b6e70cbfcf3..bd4b501b1422 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -10,6 +10,7 @@
>  #include <cstdlib>
>  #include <errno.h>
>  #include <functional>
> +#include <limits>
>
>  #include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
> @@ -151,9 +152,11 @@ int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
>         char *end;
>
>         errno = 0;
> -       int32_t value = std::strtol(value_.c_str(), &end, 10);
> +       long value = std::strtol(value_.c_str(), &end, 10);
>
> -       if ('\0' != *end || errno == ERANGE)
> +       if ('\0' != *end || errno == ERANGE ||
> +           value < std::numeric_limits<int32_t>::min() ||
> +           value > std::numeric_limits<int32_t>::max())
>                 return defaultValue;
>
>         setOk(ok, true);
> @@ -185,9 +188,11 @@ uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
>         char *end;
>
>         errno = 0;
> -       uint32_t value = std::strtoul(value_.c_str(), &end, 10);
> +       unsigned long value = std::strtoul(value_.c_str(), &end, 10);
>
> -       if ('\0' != *end || errno == ERANGE)
> +       if ('\0' != *end || errno == ERANGE ||
> +           value < std::numeric_limits<uint32_t>::min() ||
> +           value > std::numeric_limits<uint32_t>::max())
>                 return defaultValue;
>
>         setOk(ok, true);
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
index 9b6e70cbfcf3..bd4b501b1422 100644
--- a/src/libcamera/yaml_parser.cpp
+++ b/src/libcamera/yaml_parser.cpp
@@ -10,6 +10,7 @@ 
 #include <cstdlib>
 #include <errno.h>
 #include <functional>
+#include <limits>
 
 #include <libcamera/base/file.h>
 #include <libcamera/base/log.h>
@@ -151,9 +152,11 @@  int32_t YamlObject::get(const int32_t &defaultValue, bool *ok) const
 	char *end;
 
 	errno = 0;
-	int32_t value = std::strtol(value_.c_str(), &end, 10);
+	long value = std::strtol(value_.c_str(), &end, 10);
 
-	if ('\0' != *end || errno == ERANGE)
+	if ('\0' != *end || errno == ERANGE ||
+	    value < std::numeric_limits<int32_t>::min() ||
+	    value > std::numeric_limits<int32_t>::max())
 		return defaultValue;
 
 	setOk(ok, true);
@@ -185,9 +188,11 @@  uint32_t YamlObject::get(const uint32_t &defaultValue, bool *ok) const
 	char *end;
 
 	errno = 0;
-	uint32_t value = std::strtoul(value_.c_str(), &end, 10);
+	unsigned long value = std::strtoul(value_.c_str(), &end, 10);
 
-	if ('\0' != *end || errno == ERANGE)
+	if ('\0' != *end || errno == ERANGE ||
+	    value < std::numeric_limits<uint32_t>::min() ||
+	    value > std::numeric_limits<uint32_t>::max())
 		return defaultValue;
 
 	setOk(ok, true);