Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11690 closed Patch - Bug Fix (Won't Fix)

Patch to handle out-of-memory conditions in mythsystemunix.cpp - fixes Coverity 1028695

Reported by: Gary Buhrmaster <gary.buhrmaster@…> Owned by: stuartm
Priority: minor Milestone: 0.28
Component: MythTV - MythSystem Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

Static analysis determined that a check for cmdargs being NULL at cleanup was out of place (because it was already used without such checks). Further examination of the code showed that the codes did not properly handle out-of-memory allocations in strdup and malloc. While not likely, if they occured, a NULL pointer dereference could have occured in storing one of the command arg pointers, or the command could have been forked without the correct args. This was a real potential bug.

Fixes Coverity 1028695

Note that this patch appears to be huge, and somewhat unusual. This is an artifact of putting some of the code in a conditional (and the resulting indentation changes, and diff matching on random blank lines and braces).

Note that this patch depends on the patch in ticket #11607 (since it indents that code).

btw, the frees at clean up do not really need to be protected by checking for NULL since free does that internally, but since the existing code did that check, I followed the style, and made it consistent.

If this patch is accepted (style wise), I will look to (speculatively) change the mythsystemwindows.cpp file (which also does not check strdup).

github ref: https://github.com/garybuhrmaster/mythtv/commit/a70341e59962e43e85ef882c8b7d435b68500e0b github git-am ref: https://github.com/garybuhrmaster/mythtv/commit/a70341e59962e43e85ef882c8b7d435b68500e0b.patch

Change History (10)

comment:1 Changed 6 years ago by paulh

Milestone: unknown0.27

This depends on the patch in #11607 otherwise I would commit it myself

comment:2 Changed 6 years ago by paulh

Owner: set to danielk
Status: newassigned

comment:3 Changed 6 years ago by paulh

Milestone: 0.270.27.1

comment:4 Changed 5 years ago by stuartm

Milestone: 0.27.20.28
Owner: changed from danielk to stuartm
Status: assignedaccepted

comment:5 Changed 5 years ago by stuartm

Status: acceptedinfoneeded

Gary this patch no longer seems to apply cleanly to master.

comment:6 Changed 5 years ago by gary.buhrmaster@…

Correct. As you may recall, while this patch was settling in trac, you applied a specific coverity detected fix, which partially addressed a few of the issues (but not the complete list I attempted to fix). At the time, I sent a message indicating such. You stated you had overlooked my patch, but would review it later to do the merging (or just close the ticket, or revert your patch and apply mine, depending on what made the most sense).

How would you like to proceed?

comment:7 Changed 5 years ago by stuartm

Right, I'd forgotten that, sorry. I guess I'll do the latter, revert my fix and commit yours since it's more thorough.

comment:8 Changed 5 years ago by stuartm

Status: infoneededassigned

comment:9 Changed 5 years ago by stuartm

Resolution: Won't Fix
Status: assignedclosed

Having looked at it closer, it's not just my fix which prevents this being cleanly applied by at least one other by Karl Dietz. Rather than try to unpick it all I'm going to call it a day.

I'm sorry I didn't see and apply this patch when you first submitted it.

comment:10 Changed 5 years ago by gary.buhrmaster@…

Fair enough. If I end up with some future time, I may try to come up with a new patch, but probably not anytime soon (because, as I recall, it takes some time to carefully evaluate the various failure conditions and cleanups). Thanks for looking.

Note: See TracTickets for help on using tickets.