| Message ID | 20211125150500.25040-2-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/9] lavd/jack: increase buffer size for snprintf() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Anton Khirnov: > fd is local to the loop iteration, it is better to store it on stack > than modify the context. > --- > libavdevice/v4l2.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c > index b5997fba33..777867db86 100644 > --- a/libavdevice/v4l2.c > +++ b/libavdevice/v4l2.c > @@ -1033,16 +1033,17 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l > return ret; > } > while ((entry = readdir(dir))) { > + int fd = -1; > char device_name[256]; > > if (!v4l2_is_v4l_dev(entry->d_name)) > continue; > > snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name); > - if ((s->fd = device_open(ctx, device_name)) < 0) > + if ((fd = device_open(ctx, device_name)) < 0) > continue; > > - if (v4l2_ioctl(s->fd, VIDIOC_QUERYCAP, &cap) < 0) { > + if (v4l2_ioctl(fd, VIDIOC_QUERYCAP, &cap) < 0) { > ret = AVERROR(errno); > av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_QUERYCAP): %s\n", av_err2str(ret)); > goto fail; > @@ -1064,8 +1065,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l > &device_list->nb_devices, device)) < 0) > goto fail; > > - v4l2_close(s->fd); > - s->fd = -1; > + v4l2_close(fd); > continue; > > fail: > @@ -1074,9 +1074,8 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l > av_freep(&device->device_description); > av_freep(&device); > } > - if (s->fd >= 0) > - v4l2_close(s->fd); > - s->fd = -1; > + if (fd >= 0) This checks is unnecessary: One only goes to fail after the device has been successfully opened. > + v4l2_close(fd); > break; > } > closedir(dir); > 1. Commit title is wrong: This is lavd. 2. Given that there is no equivalent of avformat_alloc_output_context2() one has to use avformat_open_input() to use avdevice_list_devices(). This means that it is possible for read_header to have been called before v4l2_get_device_list() and in this case this commit fixes a file descriptor leak; it also avoids closing an invalid (-1) file descriptor v4l2_read_close(). This should be mentioned in the commit message; in fact, this is the main advantage of this commit. (I have not checked what would have happened had I actually used this context with the invalid file descriptor afterwards to read data.) - Andreas
diff --git a/libavdevice/v4l2.c b/libavdevice/v4l2.c index b5997fba33..777867db86 100644 --- a/libavdevice/v4l2.c +++ b/libavdevice/v4l2.c @@ -1033,16 +1033,17 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l return ret; } while ((entry = readdir(dir))) { + int fd = -1; char device_name[256]; if (!v4l2_is_v4l_dev(entry->d_name)) continue; snprintf(device_name, sizeof(device_name), "/dev/%s", entry->d_name); - if ((s->fd = device_open(ctx, device_name)) < 0) + if ((fd = device_open(ctx, device_name)) < 0) continue; - if (v4l2_ioctl(s->fd, VIDIOC_QUERYCAP, &cap) < 0) { + if (v4l2_ioctl(fd, VIDIOC_QUERYCAP, &cap) < 0) { ret = AVERROR(errno); av_log(ctx, AV_LOG_ERROR, "ioctl(VIDIOC_QUERYCAP): %s\n", av_err2str(ret)); goto fail; @@ -1064,8 +1065,7 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l &device_list->nb_devices, device)) < 0) goto fail; - v4l2_close(s->fd); - s->fd = -1; + v4l2_close(fd); continue; fail: @@ -1074,9 +1074,8 @@ static int v4l2_get_device_list(AVFormatContext *ctx, AVDeviceInfoList *device_l av_freep(&device->device_description); av_freep(&device); } - if (s->fd >= 0) - v4l2_close(s->fd); - s->fd = -1; + if (fd >= 0) + v4l2_close(fd); break; } closedir(dir);