From 1660a065d3752e7b058c91b5afea7e5307fe3649 Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 3 Nov 2012 15:49:25 -0500 Subject: [PATCH 1/6] typo fix, unused variable cleanup, use type in ftp_send_type, etc ffmpeg.c "and" at the end and start of the line motion.c bad spelling, to "an image" jpegutils.c hsf was set but not used netcam_ftp.c type was converted to upper case but not used, the function says it takes an I or A, as since only I is passed the end result is the same webhttpd.c consolidate the timeout to the top of the file as I needed to change it for testing --- ffmpeg.c | 2 +- jpegutils.c | 5 ++--- motion.c | 4 ++-- netcam_ftp.c | 2 +- webhttpd.c | 9 ++++++--- 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/ffmpeg.c b/ffmpeg.c index 57eeda4..86bdb8d 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -28,7 +28,7 @@ # warning Your version of FFmpeg is newer than version 0.4.8 # warning Newer versions of ffmpeg do not support MPEG1 with # warning non-standard framerate. MPEG1 will be disabled for -# warning normal video output. You can still use mpeg4 and +# warning normal video output. You can still use mpeg4 # warning and mpeg4ms which are both better in terms of size # warning and quality. MPEG1 is always used for timelapse. # warning Please read the Motion Guide for more information. diff --git a/jpegutils.c b/jpegutils.c index 9c9bc44..1622748 100644 --- a/jpegutils.c +++ b/jpegutils.c @@ -770,7 +770,7 @@ int decode_jpeg_gray_raw(unsigned char *jpeg_data, int len, unsigned int height, unsigned char *raw0, unsigned char *raw1, unsigned char *raw2) { - int numfields, hsf[3], field, yl, yc, xsl, xsc, xs, xd, hdown; + int numfields, field, yl, yc, xsl, xsc, xs, xd, hdown; unsigned int x, y, vsf[3]; JSAMPROW row0[16] = { buf0[0], buf0[1], buf0[2], buf0[3], @@ -818,8 +818,7 @@ int decode_jpeg_gray_raw(unsigned char *jpeg_data, int len, guarantee_huff_tables(&dinfo); jpeg_start_decompress (&dinfo); - hsf[0] = 1; hsf[1] = 1; hsf[2] = 1; - vsf[0]= 1; vsf[1] = 1; vsf[2] = 1; + vsf[0] = 1; vsf[1] = 1; vsf[2] = 1; /* Height match image height or be exact twice the image height. */ diff --git a/motion.c b/motion.c index 5666770..66cdf52 100644 --- a/motion.c +++ b/motion.c @@ -173,7 +173,7 @@ static void image_ring_destroy(struct context *cnt) /** * image_save_as_preview * - * This routine is called when we detect motion and want to save a image in the preview buffer + * This routine is called when we detect motion and want to save an image in the preview buffer * * Parameters: * @@ -2752,7 +2752,7 @@ int main (int argc, char **argv) SLEEP(1, 0); /* - * Calculate how many threads runnig or wants to run + * Calculate how many threads are running or wants to run * if zero and we want to finish, break out */ int motion_threads_running = 0; diff --git a/netcam_ftp.c b/netcam_ftp.c index 0a129b2..99f361a 100644 --- a/netcam_ftp.c +++ b/netcam_ftp.c @@ -795,7 +795,7 @@ int ftp_send_type(ftp_context_pointer ctxt, char type) utype = toupper(type); /* Assure transfer will be in "image" mode. */ - snprintf(buf, sizeof(buf), "TYPE I\r\n"); + snprintf(buf, sizeof(buf), "TYPE %c\r\n", utype); len = strlen(buf); res = send(ctxt->control_file_desc, buf, len, 0); diff --git a/webhttpd.c b/webhttpd.c index 20fe4e8..42fd7d8 100644 --- a/webhttpd.c +++ b/webhttpd.c @@ -16,6 +16,9 @@ #include #include +/* Timeout in seconds, used for read and write */ +const int NONBLOCK_TIMEOUT = 1; + pthread_mutex_t httpd_mutex; // This is a dummy variable use to kill warnings when not checking sscanf and similar functions @@ -197,7 +200,7 @@ static ssize_t write_nonblock(int fd, const void *buf, size_t size) struct timeval tm; fd_set fds; - tm.tv_sec = 1; /* Timeout in seconds */ + tm.tv_sec = NONBLOCK_TIMEOUT; tm.tv_usec = 0; FD_ZERO(&fds); FD_SET(fd, &fds); @@ -223,7 +226,7 @@ static ssize_t read_nonblock(int fd ,void *buf, ssize_t size) struct timeval tm; fd_set fds; - tm.tv_sec = 1; /* Timeout in seconds */ + tm.tv_sec = NONBLOCK_TIMEOUT; /* Timeout in seconds */ tm.tv_usec = 0; FD_ZERO(&fds); FD_SET(fd, &fds); @@ -2502,7 +2505,7 @@ void httpd_run(struct context **cnt) while ((client_sent_quit_message) && (!closehttpd)) { - client_socket_fd = acceptnonblocking(sd, 1); + client_socket_fd = acceptnonblocking(sd, NONBLOCK_TIMEOUT); if (client_socket_fd < 0) { if ((!cnt[0]) || (cnt[0]->finish)) { -- 2.1.4 From 3eb5e79339d713988b337d45028eaf9518578321 Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 8 Nov 2012 22:26:17 -0600 Subject: [PATCH 2/6] set motion capture thread running in the main thread I identified this while debugging, the thread was created, but hadn't yet set its state to running before the main thread checked the running variable and started another thread for the same device. That resulted in a crash. Set running in the main thread, to avoid this race condition. --- motion.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/motion.c b/motion.c index 66cdf52..e79e7d7 100644 --- a/motion.c +++ b/motion.c @@ -1108,8 +1108,6 @@ static void *motion_loop(void *arg) * is acted upon. */ unsigned long int time_last_frame = 1, time_current_frame; - - cnt->running = 1; if (motion_init(cnt) < 0) goto err; @@ -2623,11 +2621,22 @@ static void start_motion_thread(struct context *cnt, pthread_attr_t *thread_attr /* Give the thread WATCHDOG_TMO to start */ cnt->watchdog = WATCHDOG_TMO; + /* Flag it as running outside of the thread, otherwise if the main loop + * checked if it is was running before the thread set it to 1, it would + * start another thread for this device. */ + cnt->running = 1; + /* * Create the actual thread. Use 'motion_loop' as the thread * function. */ - pthread_create(&cnt->thread_id, thread_attr, &motion_loop, cnt); + if (pthread_create(&cnt->thread_id, thread_attr, &motion_loop, cnt)) { + /* thread create failed, undo running state */ + cnt->running = 0; + pthread_mutex_lock(&global_lock); + threads_running--; + pthread_mutex_unlock(&global_lock); + } } /** -- 2.1.4 From 68c671acbe67f7e4378f6a3e07113e3b599bbdec Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 8 Nov 2012 23:14:29 -0600 Subject: [PATCH 3/6] count webcontrol as a thread to avoid a crash On a SIGHUP restart the main thread waits to restart until all worker threads are finished executing, except webcontrol wasn't included. If it was still running (such as reading a web request), it would have a dangling context pointer after cnt_list was freed leading to a crash. This counts that thread in the running threads, as a termination webcontrol_finish variable to terminate it indepdent of the device thread, and creates a webcontrol_running to identify when it is running. --- CHANGELOG | 1 + CREDITS | 3 +++ motion.c | 23 ++++++++++++++++++++--- motion.h | 3 +++ webhttpd.c | 13 ++++++++++++- 5 files changed, 39 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 10ca2c2..9ac2de0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -84,6 +84,7 @@ Bugfixes * Fixed leak in vloopback. * Fixed a build of motion for some kernel version with not good videodev.h * Netcam Modulo 8 + * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter (David Fries) 3.2.12 Summary of Changes diff --git a/CREDITS b/CREDITS index 89390a7..d21549b 100644 --- a/CREDITS +++ b/CREDITS @@ -473,6 +473,9 @@ Mark Feenstra Miguel Freitas * Came up with the round robing idea. +David Fries + * Fix webhttpd race condition crash with SIGHUP, add it to running thread counter + Aaron Gage * Pointed me to the vid_mmap/int problem when calling SYNC in video.c diff --git a/motion.c b/motion.c index e79e7d7..0c7b774 100644 --- a/motion.c +++ b/motion.c @@ -340,6 +340,7 @@ static void sig_handler(int signo) while (cnt_list[++i]) { cnt_list[i]->makemovie = 1; cnt_list[i]->finish = 1; + cnt_list[i]->webcontrol_finish = 1; /* * Don't restart thread when it ends, * all threads restarts if global restart is set @@ -2747,8 +2748,21 @@ int main (int argc, char **argv) * Create a thread for the control interface if requested. Create it * detached and with 'motion_web_control' as the thread function. */ - if (cnt_list[0]->conf.webcontrol_port) - pthread_create(&thread_id, &thread_attr, &motion_web_control, cnt_list); + if (cnt_list[0]->conf.webcontrol_port) { + pthread_mutex_lock(&global_lock); + threads_running++; + /* set outside the loop to avoid thread set vs main thread check */ + cnt_list[0]->webcontrol_running = 1; + pthread_mutex_unlock(&global_lock); + if (pthread_create(&thread_id, &thread_attr, &motion_web_control, + cnt_list)) { + /* thread create failed, undo running state */ + pthread_mutex_lock(&global_lock); + threads_running--; + cnt_list[0]->webcontrol_running = 0; + pthread_mutex_unlock(&global_lock); + } + } MOTION_LOG(NTC, TYPE_ALL, NO_ERRNO, "%s: Waiting for threads to finish, pid: %d", getpid()); @@ -2770,6 +2784,9 @@ int main (int argc, char **argv) if (cnt_list[i]->running || cnt_list[i]->restart) motion_threads_running++; } + if (cnt_list[0]->conf.webcontrol_port && + cnt_list[0]->webcontrol_running) + motion_threads_running++; if (((motion_threads_running == 0) && finish) || ((motion_threads_running == 0) && (threads_running == 0))) { @@ -2829,7 +2846,7 @@ int main (int argc, char **argv) // Be sure that http control exits fine - cnt_list[0]->finish = 1; + cnt_list[0]->webcontrol_finish = 1; SLEEP(1, 0); MOTION_LOG(NTC, TYPE_ALL, NO_ERRNO, "%s: Motion terminating"); diff --git a/motion.h b/motion.h index c08d84f..567feac 100644 --- a/motion.h +++ b/motion.h @@ -370,6 +370,9 @@ struct context { volatile unsigned int restart; /* Restart the thread when it ends */ /* Is the motion thread running */ volatile unsigned int running; + /* Is the web control thread running */ + volatile unsigned int webcontrol_running; + volatile unsigned int webcontrol_finish; /* End the thread */ volatile int watchdog; pthread_t thread_id; diff --git a/webhttpd.c b/webhttpd.c index 42fd7d8..bba649e 100644 --- a/webhttpd.c +++ b/webhttpd.c @@ -2508,7 +2508,7 @@ void httpd_run(struct context **cnt) client_socket_fd = acceptnonblocking(sd, NONBLOCK_TIMEOUT); if (client_socket_fd < 0) { - if ((!cnt[0]) || (cnt[0]->finish)) { + if ((!cnt[0]) || (cnt[0]->webcontrol_finish)) { MOTION_LOG(NTC, TYPE_STREAM, NO_ERRNO, "%s: motion-httpd - Finishing"); closehttpd = 1; } @@ -2539,6 +2539,17 @@ void *motion_web_control(void *arg) { struct context **cnt = arg; httpd_run(cnt); + + /* + * Update how many threads we have running. This is done within a + * mutex lock to prevent multiple simultaneous updates to + * 'threads_running'. + */ + pthread_mutex_lock(&global_lock); + threads_running--; + cnt[0]->webcontrol_running = 0; + pthread_mutex_unlock(&global_lock); + MOTION_LOG(NTC, TYPE_STREAM, NO_ERRNO, "%s: motion-httpd thread exit"); pthread_exit(NULL); } -- 2.1.4 From 088ab0c06cdab9886bbb4010f8c5608a3ed45780 Mon Sep 17 00:00:00 2001 From: David Fries Date: Thu, 14 Aug 2014 20:43:19 -0500 Subject: [PATCH 4/6] redo watchdog termination logic, wait for the thread to cleanup I apparently have some marginal USB hardware and motion has been crashing every couple of days in the memcpy at the end of alg.c as both cnt->imgs.ref and cnt->imgs.image_virgin were NULL pointers. This was just after the main thread watchdog timer called pthread_cancel on that thread. The problem is pthread_cancel can't possibly kill a thread running on another CPU atomically, although in this case it's a single core processor and I think pthread_cancel was releasing it from the ioctl and it would then run to completition. This modifies the code to not cleanup that content's resources until that thread is no longer running. If it runs to completition a normal restart will happen, if it doesn't the main thread will check once a second until it no longer is running, cleanup and restart, but it can't cleanup with that thread running. --- motion.c | 49 ++++++++++++++++++++++++++++++++----------------- motion.h | 1 + 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/motion.c b/motion.c index 0c7b774..bd4fc2d 100644 --- a/motion.c +++ b/motion.c @@ -2804,24 +2804,39 @@ int main (int argc, char **argv) } if (cnt_list[i]->watchdog > WATCHDOG_OFF) { - cnt_list[i]->watchdog--; - - if (cnt_list[i]->watchdog == 0) { - MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, trying to do " - "a graceful restart", cnt_list[i]->threadnr); - cnt_list[i]->finish = 1; - } + if (cnt_list[i]->watchdog == WATCHDOG_KILL) { + /* if 0 then it finally did clean up (and will restart without any further action here) + * kill(, 0) == ESRCH means the thread is no longer running + * if it is no longer running with running set, then cleanup here so it can restart + */ + if(cnt_list[i]->running && pthread_kill(cnt_list[i]->thread_id, 0) == ESRCH) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: cleaning Thread %d", cnt_list[i]->threadnr); + pthread_mutex_lock(&global_lock); + threads_running--; + pthread_mutex_unlock(&global_lock); + motion_cleanup(cnt_list[i]); + cnt_list[i]->running = 0; + cnt_list[i]->finish = 0; + } + } else { + cnt_list[i]->watchdog--; + + + if (cnt_list[i]->watchdog == 0) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, trying to do " + "a graceful restart", cnt_list[i]->threadnr); + cnt_list[i]->finish = 1; + } - if (cnt_list[i]->watchdog == -60) { - MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, did NOT restart graceful," - "killing it!", cnt_list[i]->threadnr); - pthread_cancel(cnt_list[i]->thread_id); - pthread_mutex_lock(&global_lock); - threads_running--; - pthread_mutex_unlock(&global_lock); - motion_cleanup(cnt_list[i]); - cnt_list[i]->running = 0; - cnt_list[i]->finish = 0; + if (cnt_list[i]->watchdog == WATCHDOG_KILL) { + MOTION_LOG(ERR, TYPE_ALL, NO_ERRNO, "%s: Thread %d - Watchdog timeout, did NOT restart graceful," + "killing it!", cnt_list[i]->threadnr); + /* The problem is pthead_cancel might just wake up the thread so it runs to completition + * or it might not. In either case don't rip the carpet out under it by + * doing motion_cleanup until it no longer is running. + */ + pthread_cancel(cnt_list[i]->thread_id); + } } } } diff --git a/motion.h b/motion.h index 567feac..65d2698 100644 --- a/motion.h +++ b/motion.h @@ -149,6 +149,7 @@ */ #define WATCHDOG_TMO 30 /* 30 sec max motion_loop interval */ +#define WATCHDOG_KILL -60 /* -60 sec grace period before calling thread cancel */ #define WATCHDOG_OFF -127 /* Turn off watchdog, used when we wants to quit a thread */ #define CONNECTION_KO "Lost connection" -- 2.1.4 From a39ccc5e6e63a00eef600ececef7a0d2bd6ce493 Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 16 Aug 2014 00:27:28 -0500 Subject: [PATCH 5/6] get the thread out of the xioctl loop As long as errno is EINTR (and that must be the case when the USB device is still there, but the transfers are failing), the thread keeps looping running the ioctl when it isn't going to succeed, so give it access to the finish variable to only loop if the thread isn't supposed to be going away, and then keep sendig SIGVTALRM to break out of the ioctl when the thread is supposed to be exiting. With this fix the webcam was no longer crashing, which let the webcam without a problem continue to stream. --- motion.c | 10 ++++++++++ video2.c | 56 +++++++++++++++++++++++++++++--------------------------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/motion.c b/motion.c index bd4fc2d..df81ae9 100644 --- a/motion.c +++ b/motion.c @@ -356,6 +356,9 @@ static void sig_handler(int signo) break; case SIGSEGV: exit(0); + case SIGVTALRM: + printf("SIGVTALRM went off\n"); + break; } } @@ -2553,6 +2556,10 @@ static void setup_signals(struct sigaction *sig_handler_action, struct sigaction sigaction(SIGQUIT, sig_handler_action, NULL); sigaction(SIGTERM, sig_handler_action, NULL); sigaction(SIGUSR1, sig_handler_action, NULL); + + /* use SIGVTALRM as a way to break out of the ioctl, don't restart */ + sig_handler_action->sa_flags = 0; + sigaction(SIGVTALRM, sig_handler_action, NULL); } /** @@ -2817,6 +2824,9 @@ int main (int argc, char **argv) motion_cleanup(cnt_list[i]); cnt_list[i]->running = 0; cnt_list[i]->finish = 0; + } else { + /* keep sending signals so it doesn't get stuck in a blocking call */ + pthread_kill(cnt_list[i]->thread_id, SIGVTALRM); } } else { cnt_list[i]->watchdog--; diff --git a/video2.c b/video2.c index 9896edb..32d80c1 100644 --- a/video2.c +++ b/video2.c @@ -173,19 +173,20 @@ typedef struct { u32 ctrl_flags; struct v4l2_queryctrl *controls; + volatile unsigned int *finish; /* End the thread */ } src_v4l2_t; /** * xioctl */ -static int xioctl(int fd, int request, void *arg) +static int xioctl(src_v4l2_t *vid_source, int request, void *arg) { int ret; do - ret = ioctl(fd, request, arg); - while (-1 == ret && EINTR == errno); + ret = ioctl(vid_source->fd, request, arg); + while (-1 == ret && EINTR == errno && !vid_source->finish); return ret; } @@ -195,7 +196,7 @@ static int xioctl(int fd, int request, void *arg) */ static int v4l2_get_capability(src_v4l2_t * vid_source) { - if (xioctl(vid_source->fd, VIDIOC_QUERYCAP, &vid_source->cap) < 0) { + if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) { MOTION_LOG(ERR, TYPE_VIDEO, NO_ERRNO, "%s: Not a V4L2 device?"); return -1; } @@ -258,7 +259,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, input.index = IN_TV; else input.index = in; - if (xioctl(vid_source->fd, VIDIOC_ENUMINPUT, &input) == -1) { + if (xioctl(vid_source, VIDIOC_ENUMINPUT, &input) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Unable to query input %d." " VIDIOC_ENUMINPUT, if you use a WEBCAM change input value in conf by -1", input.index); @@ -274,7 +275,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, if (input.type & V4L2_INPUT_TYPE_CAMERA) MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: - CAMERA"); - if (xioctl(vid_source->fd, VIDIOC_S_INPUT, &input.index) == -1) { + if (xioctl(vid_source, VIDIOC_S_INPUT, &input.index) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error selecting input %d" " VIDIOC_S_INPUT", input.index); return -1; @@ -286,7 +287,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, * Set video standard usually webcams doesn't support the ioctl or * return V4L2_STD_UNKNOWN */ - if (xioctl(vid_source->fd, VIDIOC_G_STD, &std_id) == -1) { + if (xioctl(vid_source, VIDIOC_G_STD, &std_id) == -1) { MOTION_LOG(WRN, TYPE_VIDEO, NO_ERRNO, "%s: Device doesn't support VIDIOC_G_STD"); norm = std_id = 0; // V4L2_STD_UNKNOWN = 0 } @@ -295,7 +296,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, memset(&standard, 0, sizeof(standard)); standard.index = 0; - while (xioctl(vid_source->fd, VIDIOC_ENUMSTD, &standard) == 0) { + while (xioctl(vid_source, VIDIOC_ENUMSTD, &standard) == 0) { if (standard.id & std_id) MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: - video standard %s", standard.name); @@ -314,7 +315,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, std_id = V4L2_STD_PAL; } - if (xioctl(vid_source->fd, VIDIOC_S_STD, &std_id) == -1) + if (xioctl(vid_source, VIDIOC_S_STD, &std_id) == -1) MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error selecting standard" " method %d VIDIOC_S_STD", (int)std_id); @@ -334,7 +335,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, memset(&tuner, 0, sizeof(struct v4l2_tuner)); tuner.index = input.tuner; - if (xioctl(vid_source->fd, VIDIOC_G_TUNER, &tuner) == -1) { + if (xioctl(vid_source, VIDIOC_G_TUNER, &tuner) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: tuner %d VIDIOC_G_TUNER", tuner.index); return 0; @@ -349,7 +350,7 @@ static int v4l2_select_input(struct config *conf, struct video_dev *viddev, freq.type = V4L2_TUNER_ANALOG_TV; freq.frequency = (freq_ / 1000) * 16; - if (xioctl(vid_source->fd, VIDIOC_S_FREQUENCY, &freq) == -1) { + if (xioctl(vid_source, VIDIOC_S_FREQUENCY, &freq) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: freq %ul VIDIOC_S_FREQUENCY", freq.frequency); return 0; @@ -401,7 +402,7 @@ static int v4l2_do_set_pix_format(u32 pixformat, src_v4l2_t * vid_source, vid_source->dst_fmt.fmt.pix.pixelformat = pixformat; vid_source->dst_fmt.fmt.pix.field = V4L2_FIELD_ANY; - if (xioctl(vid_source->fd, VIDIOC_TRY_FMT, &vid_source->dst_fmt) != -1 && + if (xioctl(vid_source, VIDIOC_TRY_FMT, &vid_source->dst_fmt) != -1 && vid_source->dst_fmt.fmt.pix.pixelformat == pixformat) { MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: Testing palette %c%c%c%c (%dx%d)", pixformat >> 0, pixformat >> 8, @@ -419,7 +420,7 @@ static int v4l2_do_set_pix_format(u32 pixformat, src_v4l2_t * vid_source, *height = vid_source->dst_fmt.fmt.pix.height; } - if (xioctl(vid_source->fd, VIDIOC_S_FMT, &vid_source->dst_fmt) == -1) { + if (xioctl(vid_source, VIDIOC_S_FMT, &vid_source->dst_fmt) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error setting pixel " "format.\nVIDIOC_S_FMT: "); return -1; @@ -498,7 +499,7 @@ static int v4l2_set_pix_format(struct context *cnt, src_v4l2_t * vid_source, MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: Supported palettes:"); - while (xioctl(vid_source->fd, VIDIOC_ENUM_FMT, &fmtd) != -1) { + while (xioctl(vid_source, VIDIOC_ENUM_FMT, &fmtd) != -1) { int i; @@ -552,7 +553,7 @@ static void v4l2_set_fps(src_v4l2_t * vid_source) { setfpvid_source->parm.capture.timeperframe.numerator = 1; setfpvid_source->parm.capture.timeperframe.denominator = vid_source->fps; - if (xioctl(vid_source->fd, VIDIOC_S_PARM, setfps) == -1) + if (xioctl(vid_source, VIDIOC_S_PARM, setfps) == -1) MOTION_LOG(ERR, 1, "%s: v4l2_set_fps VIDIOC_S_PARM"); @@ -577,7 +578,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) vid_source->req.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; vid_source->req.memory = V4L2_MEMORY_MMAP; - if (xioctl(vid_source->fd, VIDIOC_REQBUFS, &vid_source->req) == -1) { + if (xioctl(vid_source, VIDIOC_REQBUFS, &vid_source->req) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error requesting buffers" " %d for memory map. VIDIOC_REQBUFS", vid_source->req.count); @@ -609,7 +610,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) buf.memory = V4L2_MEMORY_MMAP; buf.index = buffer_index; - if (xioctl(vid_source->fd, VIDIOC_QUERYBUF, &buf) == -1) { + if (xioctl(vid_source, VIDIOC_QUERYBUF, &buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error querying buffer" " %i\nVIDIOC_QUERYBUF: ", buffer_index); free(vid_source->buffers); @@ -638,7 +639,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) vid_source->buf.memory = V4L2_MEMORY_MMAP; vid_source->buf.index = buffer_index; - if (xioctl(vid_source->fd, VIDIOC_QBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_QBUF, &vid_source->buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: VIDIOC_QBUF"); return -1; } @@ -646,7 +647,7 @@ static int v4l2_set_mmap(src_v4l2_t * vid_source) type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - if (xioctl(vid_source->fd, VIDIOC_STREAMON, &type) == -1) { + if (xioctl(vid_source, VIDIOC_STREAMON, &type) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: Error starting stream." " VIDIOC_STREAMON"); return -1; @@ -667,7 +668,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) for (i = 0, count = 0; queried_ctrls[i]; i++) { queryctrl.id = queried_ctrls[i]; - if (xioctl(vid_source->fd, VIDIOC_QUERYCTRL, &queryctrl)) + if (xioctl(vid_source, VIDIOC_QUERYCTRL, &queryctrl)) continue; count++; @@ -688,7 +689,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) struct v4l2_control control; queryctrl.id = queried_ctrls[i]; - if (xioctl(vid_source->fd, VIDIOC_QUERYCTRL, &queryctrl)) + if (xioctl(vid_source, VIDIOC_QUERYCTRL, &queryctrl)) continue; memcpy(ctrl, &queryctrl, sizeof(struct v4l2_queryctrl)); @@ -700,7 +701,7 @@ static int v4l2_scan_controls(src_v4l2_t * vid_source) memset(&control, 0, sizeof (control)); control.id = queried_ctrls[i]; - xioctl(vid_source->fd, VIDIOC_G_CTRL, &control); + xioctl(vid_source, VIDIOC_G_CTRL, &control); MOTION_LOG(NTC, TYPE_VIDEO, NO_ERRNO, "%s: \t\"%s\", default %d, current %d", ctrl->name, ctrl->default_value, control.value); @@ -736,12 +737,12 @@ static int v4l2_set_control(src_v4l2_t * vid_source, u32 cid, int value) case V4L2_CTRL_TYPE_INTEGER: value = control.value = (value * (ctrl->maximum - ctrl->minimum) / 256) + ctrl->minimum; - ret = xioctl(vid_source->fd, VIDIOC_S_CTRL, &control); + ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); break; case V4L2_CTRL_TYPE_BOOLEAN: value = control.value = value ? 1 : 0; - ret = xioctl(vid_source->fd, VIDIOC_S_CTRL, &control); + ret = xioctl(vid_source, VIDIOC_S_CTRL, &control); break; default: @@ -818,6 +819,7 @@ unsigned char *v4l2_start(struct context *cnt, struct video_dev *viddev, int wid vid_source->fd = viddev->fd; vid_source->fps = cnt->conf.frame_limit; vid_source->pframe = -1; + vid_source->finish = &cnt->finish; struct config *conf = &cnt->conf; if (v4l2_get_capability(vid_source)) @@ -962,7 +964,7 @@ int v4l2_next(struct context *cnt, struct video_dev *viddev, unsigned char *map, vid_source->pframe); if (vid_source->pframe >= 0) { - if (xioctl(vid_source->fd, VIDIOC_QBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_QBUF, &vid_source->buf) == -1) { MOTION_LOG(ERR, TYPE_VIDEO, SHOW_ERRNO, "%s: VIDIOC_QBUF"); pthread_sigmask(SIG_UNBLOCK, &old, NULL); return -1; @@ -974,7 +976,7 @@ int v4l2_next(struct context *cnt, struct video_dev *viddev, unsigned char *map, vid_source->buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; vid_source->buf.memory = V4L2_MEMORY_MMAP; - if (xioctl(vid_source->fd, VIDIOC_DQBUF, &vid_source->buf) == -1) { + if (xioctl(vid_source, VIDIOC_DQBUF, &vid_source->buf) == -1) { int ret; /* * Some drivers return EIO when there is no signal, @@ -1079,7 +1081,7 @@ void v4l2_close(struct video_dev *viddev) enum v4l2_buf_type type; type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - xioctl(vid_source->fd, VIDIOC_STREAMOFF, &type); + xioctl(vid_source, VIDIOC_STREAMOFF, &type); close(vid_source->fd); vid_source->fd = -1; } -- 2.1.4 From d2a9d34acbc016563115fffc9177ebcd345f415b Mon Sep 17 00:00:00 2001 From: David Fries Date: Sat, 23 Aug 2014 14:04:22 -0500 Subject: [PATCH 6/6] fix dangling pointer cnt->current_image after resize cnt->current_image because a dangling pointer after image_ring_resize because it is pointing to cnt->imgs.image_ring which is reallocated in that routine. motion_loop will then store cnt->current_image in old_image which it can then read from. Reallocations are rare, once in init to size 1, then once to the final size. I apparently have a bad USB link and I was seeing a crash pointing to bad data, after that camera started, then had an error and crashed in process_image_ring(cnt, IMAGE_BUFFER_FLUSH); it hadn't yet resized to the normal ring buffer size. That got me trying valgrind with a ring buffer size limit of 1 which found this bug. --- motion.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/motion.c b/motion.c index df81ae9..83c11e2 100644 --- a/motion.c +++ b/motion.c @@ -133,6 +133,7 @@ static void image_ring_resize(struct context *cnt, int new_size) /* Point to the new ring */ cnt->imgs.image_ring = tmp; + cnt->current_image = NULL; cnt->imgs.image_ring_size = new_size; } @@ -167,6 +168,7 @@ static void image_ring_destroy(struct context *cnt) free(cnt->imgs.image_ring); cnt->imgs.image_ring = NULL; + cnt->current_image = NULL; cnt->imgs.image_ring_size = 0; } -- 2.1.4