Mailing List Archive

Invalid pointer de-ref in NuppelVideoRecorder.cpp
Hi Mark,

The clang-tidy program is spitting out a possible null-pointer de-
reference in NuppelVideoRecorder.cpp, function BufferIt, line 1846.

memcpy(videobuffer[act]->buffer, buf, len);

Its complaining about the "buf" variable. Backing up, this is called
from DoV4L2() on line 1567 or line 1577, which both pass the
output_buffer variable to BufferIt.

BufferIt(output_buffer, m_video_buffer_size);

Backing up a little further, the output_buffer pointer is assigned at
line 1438 inside of an "if" statement, a conditional that doesn't
control the calls to BufferIt. If this conditional is false,
output_buffer is never assigned, and is still nullptr when passed to
BufferIt.

This is in master, updated today.

David


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On Tue, 3 Dec 2019 at 06:37, David Hampton <mythtv@love2code.net> wrote:
>
> Hi Mark,
>
> The clang-tidy program is spitting out a possible null-pointer de-
> reference in NuppelVideoRecorder.cpp, function BufferIt, line 1846.
>
> memcpy(videobuffer[act]->buffer, buf, len);
>
> Its complaining about the "buf" variable. Backing up, this is called
> from DoV4L2() on line 1567 or line 1577, which both pass the
> output_buffer variable to BufferIt.
>
> BufferIt(output_buffer, m_video_buffer_size);
>
> Backing up a little further, the output_buffer pointer is assigned at
> line 1438 inside of an "if" statement, a conditional that doesn't
> control the calls to BufferIt. If this conditional is false,
> output_buffer is never assigned, and is still nullptr when passed to
> BufferIt.

David

I'm not familiar with NuppelVideoRecorder (though IMHO it should
nuked/removed/deleted as soon as possible!).

Looking at the code, buffer_output is only ever set and used when the
v4l2 pixel format is YUYV or UYVY. So the code should be safe but you
could silence the warning by adding an extra check for '&&
output_buffer' to the end of lines 1559 and 1569 - if that makes
sense.

Regards
Mark
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On Tue, 2019-12-03 at 08:08 +0000, Mark Kendall wrote:
> On Tue, 3 Dec 2019 at 06:37, David Hampton <mythtv@love2code.net>
> wrote:
> > Hi Mark,
> >
> > The clang-tidy program is spitting out a possible null-pointer de-
> > reference in NuppelVideoRecorder.cpp, function BufferIt, line 1846.
> >
> > memcpy(videobuffer[act]->buffer, buf, len);
> >
> > Its complaining about the "buf" variable. Backing up, this is
> > called
> > from DoV4L2() on line 1567 or line 1577, which both pass the
> > output_buffer variable to BufferIt.
> >
> > BufferIt(output_buffer, m_video_buffer_size);
> >
> > Backing up a little further, the output_buffer pointer is assigned
> > at
> > line 1438 inside of an "if" statement, a conditional that doesn't
> > control the calls to BufferIt. If this conditional is false,
> > output_buffer is never assigned, and is still nullptr when passed
> > to
> > BufferIt.
>
> David
>
> I'm not familiar with NuppelVideoRecorder (though IMHO it should
> nuked/removed/deleted as soon as possible!).
>
> Looking at the code, buffer_output is only ever set and used when the
> v4l2 pixel format is YUYV or UYVY. So the code should be safe but you
> could silence the warning by adding an extra check for '&&
> output_buffer' to the end of lines 1559 and 1569 - if that makes
> sense.

Sounds good. Thanks for looking at it.

David


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On Tue, Dec 3, 2019 at 8:09 AM Mark Kendall <mark.kendall@gmail.com> wrote:

> I'm not familiar with NuppelVideoRecorder (though IMHO it should
> nuked/removed/deleted as soon as possible!).

As I recall, while the NuppleVideo container may have
been used by a few projects in the past, MythTV is
now closer to being the last one standing, and pointed
to as the definitive standard reference for others.
Which is not likely to be a good place to be.

I had thought at one point there were musings to
announce the end of support in some future
version, and at that announcement recommend
people convert to a different container format to
prepare (a container only copy should be fast,
although perhaps annoying if you have a lot
of such files), and remove the various support
for the container (sort of like how XvMC was
slowly removed). But, of course, musings are
not a plan, but more like a consideration to
consider making a plan to make a plan.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On 12/4/2019 3:20 AM, Gary Buhrmaster wrote:
> On Tue, Dec 3, 2019 at 8:09 AM Mark Kendall <mark.kendall@gmail.com> wrote:
>
>> I'm not familiar with NuppelVideoRecorder (though IMHO it should
>> nuked/removed/deleted as soon as possible!).
> As I recall, while the NuppleVideo container may have
> been used by a few projects in the past, MythTV is
> now closer to being the last one standing, and pointed
> to as the definitive standard reference for others.
> Which is not likely to be a good place to be.
>
> I had thought at one point there were musings to
> announce the end of support in some future
> version, and at that announcement recommend
> people convert to a different container format to
> prepare (a container only copy should be fast,
> although perhaps annoying if you have a lot
> of such files), and remove the various support
> for the container (sort of like how XvMC was
> slowly removed). But, of course, musings are
> not a plan, but more like a consideration to
> consider making a plan to make a plan.
>
My recollection of nuppelvideo is that it was a low cpu software video
encoder for analog video recording.

This is before the new fangled multi core cpus and GPUs that you get
today and before DVB/ATSC was broadcast which does not require encoding
at all.

I still have an analog TV card or 2 here. No use any more of course and
now you just buy an analog video usb devices which encode for you.

Mark

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On Wed, Dec 04, 2019 at 07:58:47AM +1100, Mark Spieth wrote:
> On 12/4/2019 3:20 AM, Gary Buhrmaster wrote:
> > On Tue, Dec 3, 2019 at 8:09 AM Mark Kendall <mark.kendall@gmail.com> wrote:
> >
> > > I'm not familiar with NuppelVideoRecorder (though IMHO it should
> > > nuked/removed/deleted as soon as possible!).
> > As I recall, while the NuppleVideo container may have
> > been used by a few projects in the past, MythTV is
> > now closer to being the last one standing, and pointed
> > to as the definitive standard reference for others.
> > Which is not likely to be a good place to be.
> >
> > I had thought at one point there were musings to
> > announce the end of support in some future
> > version, and at that announcement recommend
> > people convert to a different container format to
> > prepare (a container only copy should be fast,
> > although perhaps annoying if you have a lot
> > of such files), and remove the various support
> > for the container (sort of like how XvMC was
> > slowly removed). But, of course, musings are
> > not a plan, but more like a consideration to
> > consider making a plan to make a plan.
> >
> My recollection of nuppelvideo is that it was a low cpu software video
> encoder for analog video recording.

I think another advantage it had was support for built-in seek tables.
I could be wrong, though. That was a long time ago.

David

> This is before the new fangled multi core cpus and GPUs that you get today
> and before DVB/ATSC was broadcast which does not require encoding at all.
>
> I still have an analog TV card or 2 here. No use any more of course and now
> you just buy an analog video usb devices which encode for you.
>
> Mark
>
> _______________________________________________
> mythtv-dev mailing list
> mythtv-dev@mythtv.org
> http://lists.mythtv.org/mailman/listinfo/mythtv-dev
> http://wiki.mythtv.org/Mailing_List_etiquette
> MythTV Forums: https://forum.mythtv.org

--
David Engel
david@istwok.net
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On Tue, Dec 3, 2019 at 9:02 PM Mark Spieth <mark@digivation.com.au> wrote:

> My recollection of nuppelvideo is that it was a low cpu software video
> encoder for analog video recording.

And I think my recollection was that nupplevideo
was essentially both an encoding, and a container
format (just like MPEG2), which means two people
can be talking about different things when using
the same term.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
Apologies - I've been meaning to follow up on this for a few days:)

My main gripe with the whole nuppel video code is that, for reasons
that were perfectly valid at the time, we backed the betamax of video
codecs:)

15 years or so down the line, we are still actively using it and
producing recordings that aren't exactly portable. Looking at the
smolt stats, it looks like there are 3 tuners using v4l2enc and 13
using v4l - not sure what the difference is, but very low numbers
regardless.

From my perspective, I'd like to do some refactoring of
avformatdecoder after 0.31 is out. That is currently heavily
complicated by the subclassing for NuppelVideoDecoder. I don't know
whether FFmpeg can decode nuppel files - and I can't find any samples
on line. We now use FFmpeg for everything else - there are currently
no private decoders and I don't really want to add any back:)

From the recorder perspective, I'd like to think it would be a
relatively straightforward change to use FFmpeg to encode to something
more sensible (FFmpeg even has its own V4L2 input device code). At
least that way we have broken the chain and even if we can't refactor
the decoding side, we have at least given ourselves an opportunity to
remove it in the future.

Hopefully that makes sense.

Regards
Mark
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Invalid pointer de-ref in NuppelVideoRecorder.cpp [ In reply to ]
On 12/9/19 9:25 AM, Mark Kendall wrote:
> >From my perspective, I'd like to do some refactoring of
> avformatdecoder after 0.31 is out. That is currently heavily
> complicated by the subclassing for NuppelVideoDecoder. I don't know
> whether FFmpeg can decode nuppel files - and I can't find any samples
> on line. We now use FFmpeg for everything else - there are currently
> no private decoders and I don't really want to add any back:)

Yes, FFMpeg can process .nuv files. However, there is currently a
serious problem with audio during playback that is documented here:

https://code.mythtv.org/trac/ticket/13459

This ticket includes a small sample file you can download.

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org