[vlc-devel] [PATCH] Support for alternative libv4l2 library (userspace drivers)

[Rémi Denis-Courmont]

    Lo,

Le 2014-07-31 22:01, Markus Rechberger a écrit :
> Attached patch adds support to access our userspace USB TV Tuner
> drivers (the analogTV part - S-Video / Composite and AnalogTV)
> directly via our interface library.
> That way VLC would also be able to access our driver interface on
> MacOSX, FreeBSD and Solaris.

Hmm? FreeBSD perhaps, but I don't think VLC has the V4L plugin on MacOS 
nor Solaris.

> It is fully compatible with the official V4L2 API, the patch will 
> have
> no effect on existing V4L2 devices.

If it were fully compatible, then it would intrinscally not need such a 
patch, so that is a bit misleading statement. In fact, I do not surmise 
why your library is not hooked as a proper libv4l2 plugin, which would 
*then* be fully compatible.

Further...

diff --git a/modules/access/v4l2/lib.c b/modules/access/v4l2/lib.c
index cf2b480..46057c5 100644
--- a/modules/access/v4l2/lib.c
+++ b/modules/access/v4l2/lib.c
@@ -25,6 +25,8 @@
  #include <pthread.h>
  #include <dlfcn.h>
  #include <unistd.h>
+#include <fcntl.h>
+#include <sys/stat.h>
  #include <sys/ioctl.h>
  #include <sys/mman.h>

@@ -34,6 +36,7 @@

  static void *v4l2_handle = NULL;
  static int (*v4l2_fd_open_) (int, int);
+static int (*v4l2_open_direct) (__const char *__file, int __oflag, 
...);

Identifiers with leading underscore are reserved.

  int (*v4l2_close) (int);
  int (*v4l2_ioctl) (int, unsigned long int, ...);
  ssize_t (*v4l2_read) (int, void *, size_t);
@@ -46,6 +49,87 @@ static int fd_open (int fd, int flags)
      return fd;
  }

+static int v4l2_check_socket() {

Incomplete prototype. This is not C++.

+        int fd;
+        char *buf=NULL;
+        int buflen=0;
+        int nread;
+        char rbuf[10240];
+        int rv;
+        struct stat statbuf;
+
+        const char *sockpath="@/de/sundtek/mediasocket";
+
+        /* FreeBSD, Solaris, MacOSX */
+        rv = stat("/tmp/.mediasocket", &statbuf);
+        if (rv == 0)
+                return 0;

Looks very much like a TOCTOU race.

Ditto for the procfs check.

+
+        fd = open("/proc/net/unix", O_RDONLY);

Missing close-on-exec.

+        if (fd >= 0) {
+                while ((nread=read(fd, rbuf, 10240))>0) {
+                        buf = (char*)realloc(buf, buflen+nread+1);

Unchecked NULL return.

+                        memcpy(&buf[buflen], rbuf, nread);
+                        buflen+=nread;
+                }
+                buf[buflen]=0;
+                if (strstr(buf, sockpath)) {

Reinventing the square wheel. There are fopen() and getline() for this.

+                        free(buf);
+			close(fd);
+                        return 0;
+                }
+		close(fd);
+                free(buf);
+        }
+        return -1;
+}
+
+static int v4l2_net_open (char *path) {
+    void *h;
+    int fd;
+
+    h = dlopen ("libmcsimple.so", RTLD_LAZY | RTLD_LOCAL);
+    if (h == NULL) {
+	/* this is the default driver location */
+	h = dlopen("/opt/lib/libmcsimple.so", RTLD_LAZY | RTLD_LOCAL);

 From a quick glance, that library is not reentrant.

+	if (h == NULL)
+            goto fallback;
+    }
+    /* no need for that one */
+    v4l2_open_direct = dlsym (h, "net_open");
+    v4l2_close = dlsym (h, "net_close");
+    v4l2_ioctl = dlsym (h, "net_ioctl");
+    v4l2_read = dlsym (h, "__net_read");
+    v4l2_mmap = dlsym (h, "net_mmap");
+    v4l2_munmap = dlsym (h, "net_munmap");

This breaks reentrancy and the memory model.
The reast of the assumes that the back-end library is constant 
throughout the lifetime of the process.

+
+    if (v4l2_open_direct != NULL && v4l2_close != NULL && v4l2_ioctl 
!= NULL
+     && v4l2_read != NULL && v4l2_mmap != NULL && v4l2_munmap != NULL)
+    {
+        v4l2_handle = h;
+        fd = v4l2_open_direct(path, O_RDWR);
+        if (fd == -1) {
+                v4l2_handle = NULL;
+                dlclose(h);
+                return -1;
+        } else {
+                return fd;
+        }
+    }
+
+    dlclose (h);
+fallback:
+    return -1;
+}
+
+int v4l2_check_device(char *path) {
+        int sk;
+        sk = v4l2_check_socket();
+        if (sk == 0)
+                return v4l2_net_open (path);
+        return -1;
+}
+
  static void v4l2_lib_load (void)
  {
      void *h = dlopen ("libv4l2.so.0", RTLD_LAZY | RTLD_LOCAL);
diff --git a/modules/access/v4l2/v4l2.c b/modules/access/v4l2/v4l2.c
index 7807a8a..beb95d9 100644
--- a/modules/access/v4l2/v4l2.c
+++ b/modules/access/v4l2/v4l2.c
@@ -466,21 +466,29 @@ int OpenDevice (vlc_object_t *obj, const char 
*path, uint32_t *restrict caps)
  {
      msg_Dbg (obj, "opening device '%s'", path);

-    int rawfd = vlc_open (path, O_RDWR);
-    if (rawfd == -1)
-    {
-        msg_Err (obj, "cannot open device '%s': %s", path,
-                 vlc_strerror_c(errno));
-        return -1;
-    }
-
-    int fd = v4l2_fd_open (rawfd, 0);
-    if (fd == -1)
-    {
-        msg_Warn (obj, "cannot initialize user-space library: %s",
-                  vlc_strerror_c(errno));
-        /* fallback to direct kernel mode anyway */
-        fd = rawfd;
+    int rawfd;
+    int fd;
+
+    fd = v4l2_check_device(path);

I don't really get why this should override even the normal existing 
V4L devices.

+
+    if (fd == -1) {
+
+	    rawfd = vlc_open (path, O_RDWR);
+	    if (rawfd == -1)
+	    {
+		msg_Err (obj, "cannot open device '%s': %s", path,
+			 vlc_strerror_c(errno));
+		return -1;
+	    }
+
+	    fd = v4l2_fd_open (rawfd, 0);
+	    if (fd == -1)
+	    {
+		msg_Warn (obj, "cannot initialize user-space library: %s",
+			  vlc_strerror_c(errno));
+		/* fallback to direct kernel mode anyway */
+		fd = rawfd;
+	    }
      }

      /* Get device capabilites */
diff --git a/modules/access/v4l2/v4l2.h b/modules/access/v4l2/v4l2.h
index 9888785..da28519 100644
--- a/modules/access/v4l2/v4l2.h
+++ b/modules/access/v4l2/v4l2.h
@@ -22,6 +22,7 @@

  /* libv4l2 functions */
  extern int v4l2_fd_open (int, int);
+extern int v4l2_check_device(char *path);
  extern int (*v4l2_close) (int);
  extern int (*v4l2_ioctl) (int, unsigned long int, ...);
  extern ssize_t (*v4l2_read) (int, void *, size_t);


-- 
Rémi Denis-Courmont