[PATCH 1/2] avformat/file: Add a specialized url_check callback for pipe protocol
Using file_check for the pipe protocol doesn't make sense. For example, for a URL like “pipe:0” it would end up checking whether the “pipe:0” file exists. This patch instead makes it check the access modes on the corresponding file descriptor using fcntl. Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/file.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/libavformat/file.c b/libavformat/file.c index 98c9e81bcb..f17d219cb2 100644 --- a/libavformat/file.c +++ b/libavformat/file.c @@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = { #if CONFIG_PIPE_PROTOCOL -static int pipe_open(URLContext *h, const char *filename, int flags) +static int pipe_get_fd(const char *filename, int flags) { - FileContext *c = h->priv_data; int fd; char *final; av_strstart(filename, "pipe:", &filename); @@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char *filename, int flags) fd = 0; } } + + return fd; +} + +static int pipe_open(URLContext *h, const char *filename, int flags) +{ + FileContext *c = h->priv_data; + int fd = pipe_get_fd(filename, flags); #if HAVE_SETMODE setmode(fd, O_BINARY); #endif @@ -398,13 +405,40 @@ static int pipe_open(URLContext *h, const char *filename, int flags) return 0; } +static int pipe_check(URLContext *h, int mask) +{ + int fd = pipe_get_fd(h->filename, mask); + int status_flags = fcntl(fd, F_GETFL); + int access; + + if (status_flags == -1) + return AVERROR(errno); + + switch (status_flags & O_ACCMODE) { + case O_RDONLY: + access = AVIO_FLAG_READ; + break; + case O_WRONLY: + access = AVIO_FLAG_WRITE; + break; + case O_RDWR: + access = AVIO_FLAG_READ | AVIO_FLAG_WRITE; + break; + default: + access = 0; + break; + } + + return access & mask; +} + const URLProtocol ff_pipe_protocol = { .name = "pipe", .url_open = pipe_open, .url_read = file_read, .url_write = file_write, .url_get_file_handle = file_get_handle, - .url_check = file_check, + .url_check = pipe_check, .priv_data_size = sizeof(FileContext), .priv_data_class = &pipe_class, .default_whitelist = "crypto,data" -- 2.37.2
Creates a UNIX pipe and then verifies that avio_check returns the right access flags for the two ends of the pipe. Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/Makefile | 1 + libavformat/tests/.gitignore | 1 + libavformat/tests/pipe.c | 101 +++++++++++++++++++++++++++++++++++ tests/fate/libavformat.mak | 5 ++ 4 files changed, 108 insertions(+) create mode 100644 libavformat/tests/pipe.c diff --git a/libavformat/Makefile b/libavformat/Makefile index f67a99f839..9c681c58c5 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -732,6 +732,7 @@ TESTPROGS-$(CONFIG_MOV_MUXER) += movenc TESTPROGS-$(CONFIG_NETWORK) += noproxy TESTPROGS-$(CONFIG_SRTP) += srtp TESTPROGS-$(CONFIG_IMF_DEMUXER) += imf +TESTPROGS-$(CONFIG_PIPE_PROTOCOL) += pipe TOOLS = aviocat \ ismindex \ diff --git a/libavformat/tests/.gitignore b/libavformat/tests/.gitignore index cdd0cce061..567d6f9e40 100644 --- a/libavformat/tests/.gitignore +++ b/libavformat/tests/.gitignore @@ -7,3 +7,4 @@ /srtp /url /seek_utils +/pipe diff --git a/libavformat/tests/pipe.c b/libavformat/tests/pipe.c new file mode 100644 index 0000000000..68540c1a8c --- /dev/null +++ b/libavformat/tests/pipe.c @@ -0,0 +1,101 @@ +/* + * Copyright (c) 2022 Neil Roberts + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <string.h> +#include <errno.h> +#include <stdlib.h> +#include <stdio.h> +#include <unistd.h> +#include "libavformat/avio.h" +#include "libavutil/error.h" + +static int check_pipe(const char *url, int mask, int expected) +{ + int flags = avio_check(url, mask); + + if (flags < 0) { + fprintf(stderr, + "avio_check for %s with mask 0x%x failed: %s\n", + url, + mask, + av_err2str(flags)); + return 0; + } + + if (flags != expected) { + fprintf(stderr, + "Wrong result returned from avio_check for %s with mask 0x%x. " + "Expected 0x%x but received 0x%x\n", + url, + mask, + flags, + expected); + return 0; + } + + return 1; +} + +int main(int argc, char **argv) +{ + int ret = 0; + int pipe_fds[2]; + char read_url[20], write_url[20]; + int check_invalid_ret; + + if (pipe(pipe_fds) == -1) { + fprintf(stderr, "error creating pipe: %s\n", strerror(errno)); + return 1; + } + + snprintf(read_url, sizeof(read_url), "pipe:%d", pipe_fds[0]); + snprintf(write_url, sizeof(write_url), "pipe:%d", pipe_fds[1]); + + if (!check_pipe(read_url, + AVIO_FLAG_READ | AVIO_FLAG_WRITE, + AVIO_FLAG_READ)) + ret = 1; + + if (!check_pipe(write_url, + AVIO_FLAG_READ | AVIO_FLAG_WRITE, + AVIO_FLAG_WRITE)) + ret = 1; + + /* Ensure that we don't get flags that we didn't ask for */ + if (!check_pipe(read_url, AVIO_FLAG_WRITE, 0)) + ret = 1; + + close(pipe_fds[0]); + close(pipe_fds[1]); + + /* An invalid fd should return EBADF */ + check_invalid_ret = avio_check(read_url, AVIO_FLAG_READ); + + if (check_invalid_ret != AVERROR(EBADF)) { + fprintf(stderr, + "avio_check on invalid FD expected to return %i " + "but %i was received\n", + AVERROR(EBADF), + check_invalid_ret); + ret = 1; + } + + return ret; +} diff --git a/tests/fate/libavformat.mak b/tests/fate/libavformat.mak index d2acb4c9e0..7a22f54c04 100644 --- a/tests/fate/libavformat.mak +++ b/tests/fate/libavformat.mak @@ -26,6 +26,11 @@ FATE_LIBAVFORMAT-$(CONFIG_IMF_DEMUXER) += fate-imf fate-imf: libavformat/tests/imf$(EXESUF) fate-imf: CMD = run libavformat/tests/imf$(EXESUF) +FATE_LIBAVFORMAT-$(CONFIG_PIPE_PROTOCOL) += fate-pipe +fate-pipe: libavformat/tests/pipe$(EXESUF) +fate-pipe: CMD = run libavformat/tests/pipe$(EXESUF) +fate-pipe: CMP = null + FATE_LIBAVFORMAT += fate-seek_utils fate-seek_utils: libavformat/tests/seek_utils$(EXESUF) fate-seek_utils: CMD = run libavformat/tests/seek_utils$(EXESUF) -- 2.37.2
On Mon, Aug 29, 2022 at 04:43:19PM +0200, Neil Roberts wrote:
Using file_check for the pipe protocol doesn't make sense. For example, for a URL like “pipe:0” it would end up checking whether the “pipe:0” file exists. This patch instead makes it check the access modes on the corresponding file descriptor using fcntl.
Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/file.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-)
breaks on mingw64 src/libavformat/file.c: In function ‘pipe_check’: src/libavformat/file.c:411:24: error: implicit declaration of function ‘fcntl’; did you mean ‘rintl’? [-Werror=implicit-function-declaration] int status_flags = fcntl(fd, F_GETFL); ^~~~~ rintl src/libavformat/file.c:411:34: error: ‘F_GETFL’ undeclared (first use in this function) int status_flags = fcntl(fd, F_GETFL); ^~~~~~~ src/libavformat/file.c:411:34: note: each undeclared identifier is reported only once for each function it appears in cc1: some warnings being treated as errors [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct answer.
Using file_check for the pipe protocol doesn't make sense. For example, for a URL like “pipe:0” it would end up checking whether the “pipe:0” file exists. This patch instead makes it check the access modes on the corresponding file descriptor using fcntl on *nix and DuplicateHandle on Windows. v2: Use DuplicateHandle on Windows to check whether the duplicated handle can have the corresponding access flags. Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/file.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/libavformat/file.c b/libavformat/file.c index 98c9e81bcb..b3f7bc9282 100644 --- a/libavformat/file.c +++ b/libavformat/file.c @@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = { #if CONFIG_PIPE_PROTOCOL -static int pipe_open(URLContext *h, const char *filename, int flags) +static int pipe_get_fd(const char *filename, int flags) { - FileContext *c = h->priv_data; int fd; char *final; av_strstart(filename, "pipe:", &filename); @@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char *filename, int flags) fd = 0; } } + + return fd; +} + +static int pipe_open(URLContext *h, const char *filename, int flags) +{ + FileContext *c = h->priv_data; + int fd = pipe_get_fd(filename, flags); #if HAVE_SETMODE setmode(fd, O_BINARY); #endif @@ -398,13 +405,74 @@ static int pipe_open(URLContext *h, const char *filename, int flags) return 0; } +static int pipe_check(URLContext *h, int mask) +{ + int fd = pipe_get_fd(h->filename, mask); + int access = 0; + +#ifdef _WIN32 + + HANDLE pipe_handle = (HANDLE) _get_osfhandle(fd); + HANDLE tmp_handle; + + if (pipe_handle == INVALID_HANDLE_VALUE) + return AVERROR(errno); + + if (mask & AVIO_FLAG_READ && + DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */ + pipe_handle, + GetCurrentProcess(), /* hTargetProcessHandle */ + &tmp_handle, + FILE_READ_DATA, + FALSE, /* bInheritHandle */ + 0 /* options */)) { + access |= AVIO_FLAG_READ; + CloseHandle(tmp_handle); + } + + if (mask & AVIO_FLAG_WRITE && + DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */ + pipe_handle, + GetCurrentProcess(), /* hTargetProcessHandle */ + &tmp_handle, + FILE_WRITE_DATA, + FALSE, /* bInheritHandle */ + 0 /* options */)) { + access |= AVIO_FLAG_WRITE; + CloseHandle(tmp_handle); + } + +#else /* _WIN32 */ + + int status_flags = fcntl(fd, F_GETFL); + + if (status_flags == -1) + return AVERROR(errno); + + switch (status_flags & O_ACCMODE) { + case O_RDONLY: + access = AVIO_FLAG_READ; + break; + case O_WRONLY: + access = AVIO_FLAG_WRITE; + break; + case O_RDWR: + access = AVIO_FLAG_READ | AVIO_FLAG_WRITE; + break; + } + +#endif /* _WIN32 */ + + return access & mask; +} + const URLProtocol ff_pipe_protocol = { .name = "pipe", .url_open = pipe_open, .url_read = file_read, .url_write = file_write, .url_get_file_handle = file_get_handle, - .url_check = file_check, + .url_check = pipe_check, .priv_data_size = sizeof(FileContext), .priv_data_class = &pipe_class, .default_whitelist = "crypto,data" -- 2.37.2
Creates a UNIX pipe and then verifies that avio_check returns the right access flags for the two ends of the pipe. v2: Add support for the Windows version of _pipe Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/Makefile | 1 + libavformat/tests/.gitignore | 1 + libavformat/tests/pipe.c | 108 +++++++++++++++++++++++++++++++++++ tests/fate/libavformat.mak | 5 ++ 4 files changed, 115 insertions(+) create mode 100644 libavformat/tests/pipe.c diff --git a/libavformat/Makefile b/libavformat/Makefile index f67a99f839..9c681c58c5 100644 --- a/libavformat/Makefile +++ b/libavformat/Makefile @@ -732,6 +732,7 @@ TESTPROGS-$(CONFIG_MOV_MUXER) += movenc TESTPROGS-$(CONFIG_NETWORK) += noproxy TESTPROGS-$(CONFIG_SRTP) += srtp TESTPROGS-$(CONFIG_IMF_DEMUXER) += imf +TESTPROGS-$(CONFIG_PIPE_PROTOCOL) += pipe TOOLS = aviocat \ ismindex \ diff --git a/libavformat/tests/.gitignore b/libavformat/tests/.gitignore index cdd0cce061..567d6f9e40 100644 --- a/libavformat/tests/.gitignore +++ b/libavformat/tests/.gitignore @@ -7,3 +7,4 @@ /srtp /url /seek_utils +/pipe diff --git a/libavformat/tests/pipe.c b/libavformat/tests/pipe.c new file mode 100644 index 0000000000..18a8551fd5 --- /dev/null +++ b/libavformat/tests/pipe.c @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2022 Neil Roberts + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <string.h> +#include <errno.h> +#include <stdlib.h> +#include <stdio.h> +#include <unistd.h> +#include "libavformat/avio.h" +#include "libavutil/error.h" + +static int check_pipe(const char *url, int mask, int expected) +{ + int flags = avio_check(url, mask); + + if (flags < 0) { + fprintf(stderr, + "avio_check for %s with mask 0x%x failed: %s\n", + url, + mask, + av_err2str(flags)); + return 0; + } + + if (flags != expected) { + fprintf(stderr, + "Wrong result returned from avio_check for %s with mask 0x%x. " + "Expected 0x%x but received 0x%x\n", + url, + mask, + expected, + flags); + return 0; + } + + return 1; +} + +int main(int argc, char **argv) +{ + int ret = 0; + int pipe_fds[2]; + char read_url[20], write_url[20]; + int check_invalid_ret; + int pipe_ret; + +#ifdef _WIN32 + pipe_ret = _pipe(pipe_fds, 1024 /* psize */, 0 /* textmode */); +#else + pipe_ret = pipe(pipe_fds); +#endif + + if (pipe_ret == -1) { + fprintf(stderr, "error creating pipe: %s\n", strerror(errno)); + return 1; + } + + snprintf(read_url, sizeof(read_url), "pipe:%d", pipe_fds[0]); + snprintf(write_url, sizeof(write_url), "pipe:%d", pipe_fds[1]); + + if (!check_pipe(read_url, + AVIO_FLAG_READ | AVIO_FLAG_WRITE, + AVIO_FLAG_READ)) + ret = 1; + + if (!check_pipe(write_url, + AVIO_FLAG_READ | AVIO_FLAG_WRITE, + AVIO_FLAG_WRITE)) + ret = 1; + + /* Ensure that we don't get flags that we didn't ask for */ + if (!check_pipe(read_url, AVIO_FLAG_WRITE, 0)) + ret = 1; + + close(pipe_fds[0]); + close(pipe_fds[1]); + + /* An invalid fd should return EBADF */ + check_invalid_ret = avio_check(read_url, AVIO_FLAG_READ); + + if (check_invalid_ret != AVERROR(EBADF)) { + fprintf(stderr, + "avio_check on invalid FD expected to return %i " + "but %i was received\n", + AVERROR(EBADF), + check_invalid_ret); + ret = 1; + } + + return ret; +} diff --git a/tests/fate/libavformat.mak b/tests/fate/libavformat.mak index d2acb4c9e0..7a22f54c04 100644 --- a/tests/fate/libavformat.mak +++ b/tests/fate/libavformat.mak @@ -26,6 +26,11 @@ FATE_LIBAVFORMAT-$(CONFIG_IMF_DEMUXER) += fate-imf fate-imf: libavformat/tests/imf$(EXESUF) fate-imf: CMD = run libavformat/tests/imf$(EXESUF) +FATE_LIBAVFORMAT-$(CONFIG_PIPE_PROTOCOL) += fate-pipe +fate-pipe: libavformat/tests/pipe$(EXESUF) +fate-pipe: CMD = run libavformat/tests/pipe$(EXESUF) +fate-pipe: CMP = null + FATE_LIBAVFORMAT += fate-seek_utils fate-seek_utils: libavformat/tests/seek_utils$(EXESUF) fate-seek_utils: CMD = run libavformat/tests/seek_utils$(EXESUF) -- 2.37.2
Bump. Is anyone interested in looking at these two patches? I’m trying to get into ffmpeg development and it would be nice to get some experience of the full process of getting a patch landed. I don’t have commit rights. Thanks in advance for any feedback. Regards, – Neil Neil Roberts <bpeeluk-at-yahoo.co.uk@ffmpeg.org> writes:
Using file_check for the pipe protocol doesn't make sense. For example, for a URL like “pipe:0” it would end up checking whether the “pipe:0” file exists. This patch instead makes it check the access modes on the corresponding file descriptor using fcntl on *nix and DuplicateHandle on Windows.
v2: Use DuplicateHandle on Windows to check whether the duplicated handle can have the corresponding access flags.
Signed-off-by: Neil Roberts <bpeeluk@yahoo.co.uk> --- libavformat/file.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 3 deletions(-)
diff --git a/libavformat/file.c b/libavformat/file.c index 98c9e81bcb..b3f7bc9282 100644 --- a/libavformat/file.c +++ b/libavformat/file.c @@ -375,9 +375,8 @@ const URLProtocol ff_file_protocol = {
#if CONFIG_PIPE_PROTOCOL
-static int pipe_open(URLContext *h, const char *filename, int flags) +static int pipe_get_fd(const char *filename, int flags) { - FileContext *c = h->priv_data; int fd; char *final; av_strstart(filename, "pipe:", &filename); @@ -390,6 +389,14 @@ static int pipe_open(URLContext *h, const char *filename, int flags) fd = 0; } } + + return fd; +} + +static int pipe_open(URLContext *h, const char *filename, int flags) +{ + FileContext *c = h->priv_data; + int fd = pipe_get_fd(filename, flags); #if HAVE_SETMODE setmode(fd, O_BINARY); #endif @@ -398,13 +405,74 @@ static int pipe_open(URLContext *h, const char *filename, int flags) return 0; }
+static int pipe_check(URLContext *h, int mask) +{ + int fd = pipe_get_fd(h->filename, mask); + int access = 0; + +#ifdef _WIN32 + + HANDLE pipe_handle = (HANDLE) _get_osfhandle(fd); + HANDLE tmp_handle; + + if (pipe_handle == INVALID_HANDLE_VALUE) + return AVERROR(errno); + + if (mask & AVIO_FLAG_READ && + DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */ + pipe_handle, + GetCurrentProcess(), /* hTargetProcessHandle */ + &tmp_handle, + FILE_READ_DATA, + FALSE, /* bInheritHandle */ + 0 /* options */)) { + access |= AVIO_FLAG_READ; + CloseHandle(tmp_handle); + } + + if (mask & AVIO_FLAG_WRITE && + DuplicateHandle(GetCurrentProcess(), /* hSourceProcessHandle */ + pipe_handle, + GetCurrentProcess(), /* hTargetProcessHandle */ + &tmp_handle, + FILE_WRITE_DATA, + FALSE, /* bInheritHandle */ + 0 /* options */)) { + access |= AVIO_FLAG_WRITE; + CloseHandle(tmp_handle); + } + +#else /* _WIN32 */ + + int status_flags = fcntl(fd, F_GETFL); + + if (status_flags == -1) + return AVERROR(errno); + + switch (status_flags & O_ACCMODE) { + case O_RDONLY: + access = AVIO_FLAG_READ; + break; + case O_WRONLY: + access = AVIO_FLAG_WRITE; + break; + case O_RDWR: + access = AVIO_FLAG_READ | AVIO_FLAG_WRITE; + break; + } + +#endif /* _WIN32 */ + + return access & mask; +} + const URLProtocol ff_pipe_protocol = { .name = "pipe", .url_open = pipe_open, .url_read = file_read, .url_write = file_write, .url_get_file_handle = file_get_handle, - .url_check = file_check, + .url_check = pipe_check, .priv_data_size = sizeof(FileContext), .priv_data_class = &pipe_class, .default_whitelist = "crypto,data" -- 2.37.2
_______________________________________________ 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".
participants (2)
-
Michael Niedermayer -
Neil Roberts