diff mbox series

[FFmpeg-devel,1/2] avcodec/v4l2_m2m: Avoid using intermediate buffer

Message ID 20200303042006.6370-1-andreas.rheinhardt@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/2] avcodec/v4l2_m2m: Avoid using intermediate buffer
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 3, 2020, 4:20 a.m. UTC
Up until now, v4l2_m2m would write via snprintf() into an intermediate
buffer and then copy from there (via strncpy()) to the end buffer. This
commit changes this by removing the intermediate buffer.

The call to strncpy() was actually of the form strncpy(dst, src,
strlen(src) + 1) which is unsafe in general, but safe in this instance
because dst and src were both of the same size and src was a proper
zero-terminated string. But this nevertheless led to a compiler warning
"‘strncpy’ specified bound depends on the length of the source argument
[-Wstringop-overflow=]" in GCC 9.2. strlen() was unnecessary anyway.

Reviewed-by: Andriy Gelman <andriy.gelman@gmail.com>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Thanks to Andriy for testing and reviewing.

 libavcodec/v4l2_m2m.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Andreas Rheinhardt March 10, 2020, 5:37 a.m. UTC | #1
Andreas Rheinhardt:
> Up until now, v4l2_m2m would write via snprintf() into an intermediate
> buffer and then copy from there (via strncpy()) to the end buffer. This
> commit changes this by removing the intermediate buffer.
> 
> The call to strncpy() was actually of the form strncpy(dst, src,
> strlen(src) + 1) which is unsafe in general, but safe in this instance
> because dst and src were both of the same size and src was a proper
> zero-terminated string. But this nevertheless led to a compiler warning
> "‘strncpy’ specified bound depends on the length of the source argument
> [-Wstringop-overflow=]" in GCC 9.2. strlen() was unnecessary anyway.
> 
> Reviewed-by: Andriy Gelman <andriy.gelman@gmail.com>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Thanks to Andriy for testing and reviewing.
> 
>  libavcodec/v4l2_m2m.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
> index 2d21f910bc..e48b3a8ccf 100644
> --- a/libavcodec/v4l2_m2m.c
> +++ b/libavcodec/v4l2_m2m.c
> @@ -358,7 +358,6 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>  {
>      int ret = AVERROR(EINVAL);
>      struct dirent *entry;
> -    char node[PATH_MAX];
>      DIR *dirp;
>  
>      V4L2m2mContext *s = priv->context;
> @@ -372,9 +371,8 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>          if (strncmp(entry->d_name, "video", 5))
>              continue;
>  
> -        snprintf(node, sizeof(node), "/dev/%s", entry->d_name);
> -        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", node);
> -        strncpy(s->devname, node, strlen(node) + 1);
> +        snprintf(s->devname, sizeof(s->devname), "/dev/%s", entry->d_name);
> +        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", s->devname);
>          ret = v4l2_probe_driver(s);
>          if (!ret)
>              break;
> @@ -389,7 +387,7 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>          return ret;
>      }
>  
> -    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", node);
> +    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", s->devname);
>  
>      return v4l2_configure_contexts(s);
>  }
> 

Ping for this patchset.

- Andreas
Andriy Gelman March 10, 2020, 6:11 p.m. UTC | #2
On Tue, 10. Mar 06:37, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
> > Up until now, v4l2_m2m would write via snprintf() into an intermediate
> > buffer and then copy from there (via strncpy()) to the end buffer. This
> > commit changes this by removing the intermediate buffer.
> > 
> > The call to strncpy() was actually of the form strncpy(dst, src,
> > strlen(src) + 1) which is unsafe in general, but safe in this instance
> > because dst and src were both of the same size and src was a proper
> > zero-terminated string. But this nevertheless led to a compiler warning
> > "‘strncpy’ specified bound depends on the length of the source argument
> > [-Wstringop-overflow=]" in GCC 9.2. strlen() was unnecessary anyway.
> > 
> > Reviewed-by: Andriy Gelman <andriy.gelman@gmail.com>
> > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> > ---
> > Thanks to Andriy for testing and reviewing.
> > 
> >  libavcodec/v4l2_m2m.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
> > index 2d21f910bc..e48b3a8ccf 100644
> > --- a/libavcodec/v4l2_m2m.c
> > +++ b/libavcodec/v4l2_m2m.c
> > @@ -358,7 +358,6 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
> >  {
> >      int ret = AVERROR(EINVAL);
> >      struct dirent *entry;
> > -    char node[PATH_MAX];
> >      DIR *dirp;
> >  
> >      V4L2m2mContext *s = priv->context;
> > @@ -372,9 +371,8 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
> >          if (strncmp(entry->d_name, "video", 5))
> >              continue;
> >  
> > -        snprintf(node, sizeof(node), "/dev/%s", entry->d_name);
> > -        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", node);
> > -        strncpy(s->devname, node, strlen(node) + 1);
> > +        snprintf(s->devname, sizeof(s->devname), "/dev/%s", entry->d_name);
> > +        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", s->devname);
> >          ret = v4l2_probe_driver(s);
> >          if (!ret)
> >              break;
> > @@ -389,7 +387,7 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
> >          return ret;
> >      }
> >  
> > -    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", node);
> > +    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", s->devname);
> >  
> >      return v4l2_configure_contexts(s);
> >  }
> > 
> 
> Ping for this patchset.
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

lgtm

Thanks,
James Almer March 10, 2020, 6:18 p.m. UTC | #3
On 3/10/2020 3:11 PM, Andriy Gelman wrote:
> On Tue, 10. Mar 06:37, Andreas Rheinhardt wrote:
>> Andreas Rheinhardt:
>>> Up until now, v4l2_m2m would write via snprintf() into an intermediate
>>> buffer and then copy from there (via strncpy()) to the end buffer. This
>>> commit changes this by removing the intermediate buffer.
>>>
>>> The call to strncpy() was actually of the form strncpy(dst, src,
>>> strlen(src) + 1) which is unsafe in general, but safe in this instance
>>> because dst and src were both of the same size and src was a proper
>>> zero-terminated string. But this nevertheless led to a compiler warning
>>> "‘strncpy’ specified bound depends on the length of the source argument
>>> [-Wstringop-overflow=]" in GCC 9.2. strlen() was unnecessary anyway.
>>>
>>> Reviewed-by: Andriy Gelman <andriy.gelman@gmail.com>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> Thanks to Andriy for testing and reviewing.
>>>
>>>  libavcodec/v4l2_m2m.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
>>> index 2d21f910bc..e48b3a8ccf 100644
>>> --- a/libavcodec/v4l2_m2m.c
>>> +++ b/libavcodec/v4l2_m2m.c
>>> @@ -358,7 +358,6 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>>>  {
>>>      int ret = AVERROR(EINVAL);
>>>      struct dirent *entry;
>>> -    char node[PATH_MAX];
>>>      DIR *dirp;
>>>  
>>>      V4L2m2mContext *s = priv->context;
>>> @@ -372,9 +371,8 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>>>          if (strncmp(entry->d_name, "video", 5))
>>>              continue;
>>>  
>>> -        snprintf(node, sizeof(node), "/dev/%s", entry->d_name);
>>> -        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", node);
>>> -        strncpy(s->devname, node, strlen(node) + 1);
>>> +        snprintf(s->devname, sizeof(s->devname), "/dev/%s", entry->d_name);
>>> +        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", s->devname);
>>>          ret = v4l2_probe_driver(s);
>>>          if (!ret)
>>>              break;
>>> @@ -389,7 +387,7 @@ int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
>>>          return ret;
>>>      }
>>>  
>>> -    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", node);
>>> +    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", s->devname);
>>>  
>>>      return v4l2_configure_contexts(s);
>>>  }
>>>
>>
>> Ping for this patchset.
>>
>> - Andreas
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> lgtm
> 
> Thanks,

Applied patch 1/2, which i assume is the one you approved.
diff mbox series

Patch

diff --git a/libavcodec/v4l2_m2m.c b/libavcodec/v4l2_m2m.c
index 2d21f910bc..e48b3a8ccf 100644
--- a/libavcodec/v4l2_m2m.c
+++ b/libavcodec/v4l2_m2m.c
@@ -358,7 +358,6 @@  int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
 {
     int ret = AVERROR(EINVAL);
     struct dirent *entry;
-    char node[PATH_MAX];
     DIR *dirp;
 
     V4L2m2mContext *s = priv->context;
@@ -372,9 +371,8 @@  int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
         if (strncmp(entry->d_name, "video", 5))
             continue;
 
-        snprintf(node, sizeof(node), "/dev/%s", entry->d_name);
-        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", node);
-        strncpy(s->devname, node, strlen(node) + 1);
+        snprintf(s->devname, sizeof(s->devname), "/dev/%s", entry->d_name);
+        av_log(s->avctx, AV_LOG_DEBUG, "probing device %s\n", s->devname);
         ret = v4l2_probe_driver(s);
         if (!ret)
             break;
@@ -389,7 +387,7 @@  int ff_v4l2_m2m_codec_init(V4L2m2mPriv *priv)
         return ret;
     }
 
-    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", node);
+    av_log(s->avctx, AV_LOG_INFO, "Using device %s\n", s->devname);
 
     return v4l2_configure_contexts(s);
 }