NetBSD Problem Report #58163

From www@netbsd.org  Wed Apr 17 17:04:10 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 76B441A9238
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 17 Apr 2024 17:04:10 +0000 (UTC)
Message-Id: <20240417170408.D6C021A923A@mollari.NetBSD.org>
Date: Wed, 17 Apr 2024 17:04:08 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: dead branches in dtv_demux_close
X-Send-Pr-Version: www-1.0

>Number:         58163
>Category:       kern
>Synopsis:       dead branches in dtv_demux_close
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Apr 17 17:05:00 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetDTV Foundation
>Environment:
>Description:
dtv_demux_close, a struct fileops::fo_close function, has a branch for the event of dtv_demux_stop failure:

    341 	/* If the demux is still running, stop it */
    342 	if (demux->dd_running) {
    343 		error = dtv_demux_stop(demux);
    344 		if (error)
    345 			return error;
    346 	}

This early return skips over releasing resources:

    353 	mutex_destroy(&demux->dd_lock);
    354 	cv_destroy(&demux->dd_section_cv);
    355 	kmem_free(demux, sizeof(*demux));
    356 
    357 	/* Update the global device open count */
    358 	dtv_common_close(sc);

https://nxr.netbsd.org/xref/src/sys/dev/dtv/dtv_demux.c?r=1.11#341

This would be a bug if dtv_demux_stop could fail, because .fo_close is final; there will never be a second call to .fo_close, so any failure here is necessarily a memory leak.

But dtv_demux_stop only fails if dtv_device_stop_transfer fails, and every struct dtv_hw_if::stop_transfer function unconditionally returns zero:

https://nxr.netbsd.org/xref/src/sys/dev/pci/coram.c?r=1.20#716
https://nxr.netbsd.org/xref/src/sys/dev/pci/cxdtv.c?r=1.22#637
https://nxr.netbsd.org/xref/src/sys/dev/usb/auvitek_dtv.c?r=1.10#262
https://nxr.netbsd.org/xref/src/sys/dev/usb/emdtv_dtv.c?r=1.17#343

Rather than maintain these buggy dead branches, .stop_transfer should just be made to return void and all this logic be made unconditional.
>How-To-Repeat:
code inspection
>Fix:
1. make struct dtv_hw_if::stop_transfer return void
2. prune dead branches
3. review this code for MP-safety while here, access to dd_running looks dodgy -- maybe dtv_demux_close should just call dtv_demux_stop unconditionally

Home	PR Database Search