Double free() issue caused by nvvidconv

[zhang.dongchuan]

Dear Nvidia,

I met a signal 6 issue during caps negotiation of nvvidconv and downstream elements.

I looked into the source code of nvvidconv and found the problematic code in gst_nvvconv_fixate_caps():

if (have_nvfeature) {
for (i = 0; i < index; i++) {
gst_caps_remove_structure (othercaps, i);
}
}

othercaps = gst_caps_truncate (othercaps);

It’s a wrong way to call gst_caps_remove_structure in a for loop with increasing order of index.
It should be called in reverse order.
Because every call of gst_caps_remove_structure would internally call g_array_remove_index which implements removal of elements by memmove（copying memory from [index+1, last_index] to [index, last_index-1 .

In my case, ‘othercaps’ has size of 4 and all of them have nv-mem feature.
Let’s assume the memory looks like: |0|1|2|3|x|
On 1st round of loop, caps of index 0 could be removed correctly.
Length of ‘othercaps’ would be 3.
Then memory would be like: |1|2|3|3|x|

But on 2nd round of loop, caps of index 2 would be removed as I explained above.
Length of ‘othercaps’ would be 2.
Then memory would be like: |1|3|3|3|x|

On 3rd round of loop, caps of index 3 would be removed due to a small bug in gst_caps_remove_structure()
(gstcaps.c, line677,
g_return_if_fail (idx <= gst_caps_get_size (caps)); Here should be ‘<’ instead of ‘<=’
)

So now ‘othercaps’ has length of 2 with index 1 and dangling index 3.
The later call of ‘gst_caps_truncate’ would try to free the index 3 again which would cause the signal 6 issue.

[zhang.dongchuan]

I tried to fix the issue with following change.

One point I am not very sure is that:
In original code, it would remove all caps prior to the very last caps with NVMM feature.
But in my fix, I would only remove all caps prior to the very 1st caps with NVMM feature. (I added a break in the loop)
I don’t know if this difference could cause any side effect.
(I changed this behavior because I found that Gstreamer would normally prefer 1st caps if multiple caps provided)

Could you please help double confirm if my fix is correct?
Thanks a lot.

[kayccc]

I will forward to our internal team to do the investigation and provide suggestions soon. Thanks

[DaneLLL]

Hi,
Thanks for reporting it and share the patch. This may be the case we don’t test well enough. Would need your help to share a method so that we can replicate the issue first and do further check.

[zhang.dongchuan]

I occasionally found a very simple way to reproduce this issue.
There were 8 caps provided to nvvidconv.
Please try this pipeline:
GST_DEBUG=5 gst-launch-1.0 videotestsrc ! nvvidconv ! nv3dsink

And you will see following trace:
0:00:00.648920749 12694 0x558df460f0 DEBUG basetransform gstbasetransform.c:1165:gst_base_transform_find_transform: calling fixate_caps for video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)RGBA; video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)BGRx; video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)I420; video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)NV12; video/x-raw, width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)Y42B; video/x-raw(memory:NVMM), width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)NV12; video/x-raw(memory:NVMM), width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)I420; video/x-raw(memory:NVMM), width=(int)[ 1, 2147483647 ], height=(int)[ 1, 2147483647 ], framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)[ 1/2147483647, 2147483647/1 ], interlace-mode=(string)progressive, format=(string)RGBA using caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, multiview-mode=(string)mono, format=(string)RGBA, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive on pad nvvconv0:src

(gst-launch-1.0:12694): GLib-CRITICAL **: 18:33:42.415: g_array_remove_index: assertion ‘index_ < array->len’ failed

(gst-launch-1.0:12694): GStreamer-CRITICAL **: 18:33:42.415: gst_caps_remove_structure: assertion ‘idx <= gst_caps_get_size (caps)’ failed

(gst-launch-1.0:12694): GStreamer-CRITICAL **: 18:33:42.415: gst_caps_remove_structure: assertion ‘idx <= gst_caps_get_size (caps)’ failed

(gst-launch-1.0:12694): GStreamer-CRITICAL **: 18:33:42.415: gst_structure_set_parent_refcount: assertion ‘refcount != NULL’ failed

(gst-launch-1.0:12694): GStreamer-CRITICAL **: 18:33:42.415: gst_caps_features_set_parent_refcount: assertion ‘refcount != NULL’ failed

free(): invalid pointer

And I also tried my fix.
No such issue met.

[zhang.dongchuan]

Hi kayccc,

Sorry to push you but it’s quite urgent for us to fix this issue ASAP.
Is there any progress now?
Thanks.

[DaneLLL]

Hi,
We have checked and the pipeline looks invalid. It should be run like

gst-launch-1.0 videotestsrc ! nvvidconv ! 'video/x-raw(memory:NVMM)' ! nv3dsink

The nvvidconv does not support:

video/x-raw ! nvvidconv ! video/x-raw

So need to specify NVMM buffer to source pad:

video/x-raw ! nvvidconv ! video/x-raw(memory:NVMM)

The patch may be required in specific use-cases you are running. For further confirming the patch is good, please try to run your application with

GST_DEBUG=“GST_TRACER:7” GST_TRACERS=“leaks”

To make sure it does not report any leaks after applying the patch.

[zhang.dongchuan]

Hi DaneLLL,

I am afraid I can’t agree with your comment on the caps filter part.
I know that nvvidconv does not support converting video/x-raw to video/x-raw.
But the nv3dsink supports both video/x-raw and video/x-raw(memory:NVMM).
If we look into the source code of gst_nvvconv_fixate_caps, we will see that nvvidconv will always prefer video/x-raw(memory:NVMM) to video/x-raw.
So there is no need to specify the ‘video/x-raw(memory:NVMM)’ actually.
I can confirm this point by applying my patch, and I can see the pipeline could run smoothly.

[zhang.dongchuan]

Hi DaneLLL,

Another example is gst-launch-1.0 videotestsrc ! nvvidconv ! nvoverlaysink
This pipeline has no ‘nvmm’ caps specified and can run smoothly without my fix.
There are only 2 caps returned by nvoverlaysink which would not cause issue in nvvidconv.
(As I explained earlier, the nvvidconv issue could only happen with 2N (N>=2) caps returned by downstream element.)

[zhang.dongchuan]

Hi DaneLLL,

As I posted earlier , what I concerned is that : (let me paste here)
In original code, it would remove all caps prior to the very last caps with NVMM feature.
But in my fix, I would only remove all caps prior to the very 1st caps with NVMM feature. (I added a break in the loop)
I don’t know if this difference could cause any side effect.
(I changed this behavior because I found that Gstreamer would normally prefer 1st caps if multiple caps provided)

So I’d like Nvidia expert give me some clues here if my change is good enough to go, not only memory leak part.
Thanks a lot.

[DaneLLL]

Hi,
We think it is fine to specify the caps in sink pad and source pad of nvvidconv plugin, so the default plugin looks OK. Since the plugins are open source and the patch looks fine, you may apply it for your use-case and rebuild the plugin.