Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#9386 closed defect (fixed)

Potential error(e.g., resource leak, deadlock) due to the unreleased lock in function avf_read_packet in FFmpeg/libavdevice/avfoundation.m

Reported by: cyeaa Owned by:
Priority: normal Component: avdevice
Version: git-master Keywords: deadlock leak avfoundation
Cc: cyeaa Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Hi,developers,thank you for your checking.The lock ctx->frame_lock may be not released correctly if the method avf_read_packe returns at eleven error handling branches(line 1067, line 1071, line 1098, line 1104, line 1108, line 1112, line 1131, line 1142, line 1162, line 1172 and line 1174). The relevant code is listed below and the problematic place is commented.

source code link:
https://github.com/FFmpeg/FFmpeg/blob/master/libavdevice/avfoundation.m#L1053

FFmpeg/libavdevice/avfoundation.m:

  static void lock_frames(AVFContext* ctx)
  {
        pthread_mutex_lock(&ctx->frame_lock);
  }

  static void unlock_frames(AVFContext* ctx)
  {
        pthread_mutex_unlock(&ctx->frame_lock);
  }
  
  static int avf_read_packet(AVFormatContext *s, AVPacket *pkt)
  {
    AVFContext* ctx = (AVFContext*)s->priv_data;

    do {
        CVImageBufferRef image_buffer;
        CMBlockBufferRef block_buffer;
        lock_frames(ctx);

        if (ctx->current_frame != nil) {
            ...;

            if (image_buffer != nil) {
                length = (int)CVPixelBufferGetDataSize(image_buffer);
            } else if (block_buffer != nil) {
                length = (int)CMBlockBufferGetDataLength(block_buffer);
            } else  {
                return AVERROR(EINVAL); //line 1067,lock leak: ctx->frame_lock is not released before return
            }

            if (av_new_packet(pkt, length) < 0) {
                return AVERROR(EIO); //line 1071,lock leak: ctx->frame_lock is not released before return
            }

            ...;

            if (status < 0)
                return status; //line 1098,lock leak: ctx->frame_lock is not released before return
        } else if (ctx->current_audio_frame != nil) {
            CMBlockBufferRef block_buffer = CMSampleBufferGetDataBuffer(ctx->current_audio_frame);
            int block_buffer_size         = CMBlockBufferGetDataLength(block_buffer);

            if (!block_buffer || !block_buffer_size) {
                return AVERROR(EIO); //line 1104,lock leak: ctx->frame_lock is not released before return
            }

            if (ctx->audio_non_interleaved && block_buffer_size > ctx->audio_buffer_size) {
                return AVERROR_BUFFER_TOO_SMALL; //line 1108,lock leak: ctx->frame_lock is not released before return
            }

            if (av_new_packet(pkt, block_buffer_size) < 0) {
                return AVERROR(EIO); //line 1112,lock leak: ctx->frame_lock is not released before return
            }

            ...;

            if (ctx->audio_non_interleaved) {
                int sample, c, shift, num_samples;

                OSStatus ret = CMBlockBufferCopyDataBytes(block_buffer, 0, pkt->size, ctx->audio_buffer);
                if (ret != kCMBlockBufferNoErr) {
                    return AVERROR(EIO); //line 1131,lock leak: ctx->frame_lock is not released before return
                }

                num_samples = pkt->size / (ctx->audio_channels * (ctx->audio_bits_per_sample >> 3));

                // transform decoded frame into output format
                #define INTERLEAVE_OUTPUT(bps)                                         
                {                               
                                        ...;
                    if (!src) return AVERROR(EIO); //line 1142,lock leak: ctx->frame_lock is not released before return
                                        ...;
                }

                                ...;
            } else {
                OSStatus ret = CMBlockBufferCopyDataBytes(block_buffer, 0, pkt->size, pkt->data);
                if (ret != kCMBlockBufferNoErr) {
                    return AVERROR(EIO); //line 1162,lock leak: ctx->frame_lock is not released before return
                }
            }

            CFRelease(ctx->current_audio_frame);
            ctx->current_audio_frame = nil;
        } else {
            pkt->data = NULL;
            unlock_frames(ctx);
            if (ctx->observed_quit) {
                return AVERROR_EOF; //line 1172,lock leak: ctx->frame_lock is not released before return
            } else {
                return AVERROR(EAGAIN); //line 1174,lock leak: ctx->frame_lock is not released before return
            }
        }

        unlock_frames(ctx);
    } while (!pkt->data);

    return 0;
  }

Fixing suggestion:
add unlock_frames(ctx); at all the above branches.

Change History (2)

comment:1 by Thilo Borgmann, 3 years ago

Resolution: fixed
Status: newclosed

comment:2 by Carl Eugen Hoyos, 20 months ago

Keywords: avfoundation added; lock removed
Note: See TracTickets for help on using tickets.