Bug 33201
| Summary: | net/net_osdep.c:if_name is broken | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Robert Watson <rwatson> |
| Component: | kern | Assignee: | Brooks Davis <brooks> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | ||
| Priority: | Normal | ||
| Version: | 5.0-CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
Slight update on this: I misread the function, it's evil due to its use of MAXNUMBUF rather than its use of stack space. Comments about IFNAMSIZE still apply. If we want to formally maintain an interface name string somewhere, I wouldn't mind, but it should probably be in struct ifnet. It might reduce the number of potential if_name/if_unit/string-based bugs substantially. Robert N M Watson FreeBSD Core Team, TrustedBSD Project robert@fledge.watson.org NAI Labs, Safeport Network Services On Wed, 26 Dec 2001, Robert Watson wrote: > > >Number: 33201 > >Category: kern > >Synopsis: net/net_osdep.c:if_name is broken > >Confidential: no > >Severity: serious > >Priority: low > >Responsible: freebsd-bugs > >State: open > >Quarter: > >Keywords: > >Date-Required: > >Class: sw-bug > >Submitter-Id: current-users > >Arrival-Date: Wed Dec 26 08:00:01 PST 2001 > >Closed-Date: > >Last-Modified: > >Originator: Robert Watson > >Release: FreeBSD 5.0-CURRENT i386 > >Organization: > NAI Labs > >Environment: > System: > > FreeBSD curry.decoverly.watson.org 5.0-CURRENT FreeBSD 5.0-CURRENT #0: Sun Dec 23 23:15:06 EST 2001 rwatson@curry.decoverly.watson.org:/usr/obj/usr/src/sys/BKTR i386 > > >Description: > > While it doesn't actually currently result in a failure that I can find, > if_name() is written in a very failure-prone style: > > const char * > if_name(ifp) > struct ifnet *ifp; > { > #define MAXNUMBUF 8 > static char nam[MAXNUMBUF][IFNAMSIZ + 10]; /*enough?*/ > static int ifbufround = 0; > char *cp; > > ifbufround = (ifbufround + 1) % MAXNUMBUF; > cp = nam[ifbufround]; > > snprintf(cp, IFNAMSIZ + 10, "%s%d", ifp->if_name, ifp->if_unit); > return((const char *)cp); > #undef MAXNUMBUF > } > > Returning pointers into the stack space of a function that is exiting > is *never* a good idea. > > Also, this code suffers from a continuing confusion about the possible > size of interface name strings that seems widespread through the sack. > IFNAMSIZ should either be sufficient and used everywhere, or be fixed. > As it stands, I found several hard-coded variants on the them, and > the ifunit() code to build a device name also looked fairly suspect. > > >How-To-Repeat: > >Fix: > >Release-Note: > >Audit-Trail: > >Unformatted: > > To Unsubscribe: send mail to majordomo@FreeBSD.org > with "unsubscribe freebsd-bugs" in the body of the message > On Wed, 26 Dec 2001, Robert Watson wrote:
> Slight update on this: I misread the function, it's evil due to its
> use of MAXNUMBUF rather than its use of stack space. Comments about
> IFNAMSIZE still apply. If we want to formally maintain an interface name
The multiple static buffer hack is even worse than a single static
buffer in threaded and/or interruptible environments. inet_ntoa() in
the kernel an example of an old BAD interface that is implemented using
the latter. I think if_name() and inet_ntoa() are only used in kernel
printfs, so the effects of races are somewhat limited. With a static
buffer that always has a NUL at the end, the buffer contents may be
garbage. With the multiple static buffer hack, there are also races
bumping the buffer index, but for if_name() the buffer index is bumped
in a fairly fail-safe way which might prevent it becoming invalid if
the compiler doesn't do too many optimizations.
Bruce
State Changed From-To: open->feedback It looks like this was resolved with the if_xname commit. Can this PR be closed? On Wed, Aug 25, 2004 at 08:59:01PM +0000, Tilman Linneweh wrote:
> Synopsis: net/net_osdep.c:if_name is broken
>
> State-Changed-From-To: open->feedback
> State-Changed-By: arved
> State-Changed-When: Wed Aug 25 20:56:22 GMT 2004
> State-Changed-Why:
> It looks like this was resolved with the if_xname commit.
> Can this PR be closed?
Unless we care about fixing it in stable, we can close this. if_name()
is correct other then a couple style bugs.
We might want to just nuke net_osdep.c and replace if_name() with:
#define if_name(ifp) ((ifp)->if_xname)
This was over a year ago in NetBSD.
-- Brooks
--
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4
State Changed From-To: feedback->patched Correct state. It seems to me that this can be MFCed to RELENG_5 and perhaps to RELENG_4 too (not so sure about the latter..). Thank you Best Regards -- Rionda aka Matteo Riondato Disinformato per default G.U.F.I. Staff Member (http://www.gufi.org) FreeSBIE Developer (http://www.freesbie.org) State Changed From-To: patched->closed I've MFCd this to 5.x. There's no trivial solution to the problem on 4.x and it's not a sufficently serious issue there that we actually need to fix it. gNATS: Enter the reason for changing this PR's state here. Responsible Changed From-To: freebsd-bugs->brooks I removed the function in favor of a macro on 4 and 5. |
While it doesn't actually currently result in a failure that I can find, if_name() is written in a very failure-prone style: const char * if_name(ifp) struct ifnet *ifp; { #define MAXNUMBUF 8 static char nam[MAXNUMBUF][IFNAMSIZ + 10]; /*enough?*/ static int ifbufround = 0; char *cp; ifbufround = (ifbufround + 1) % MAXNUMBUF; cp = nam[ifbufround]; snprintf(cp, IFNAMSIZ + 10, "%s%d", ifp->if_name, ifp->if_unit); return((const char *)cp); #undef MAXNUMBUF } Returning pointers into the stack space of a function that is exiting is *never* a good idea. Also, this code suffers from a continuing confusion about the possible size of interface name strings that seems widespread through the sack. IFNAMSIZ should either be sufficient and used everywhere, or be fixed. As it stands, I found several hard-coded variants on the them, and the ifunit() code to build a device name also looked fairly suspect.