| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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
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,
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 --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); }