Reentrancy or not with this netbsd code

Question

I am studying on "reading code" by reading pieces of NetBSD source code.
(for whoever is interested, it's < Code Reading: The Open Source Perspective > I'm reading)

And I found this function:

/* convert IP address to a string, but not into a single buffer
*/
char *
naddr_ntoa(naddr a)
{
#define NUM_BUFS 4
    static int bufno;
    static struct {
    char str[16];   /* xxx.xxx.xxx.xxx\0 */
    } bufs[NUM_BUFS];
    char *s;
    struct in_addr addr;

    addr.s_addr = a;
    strlcpy(bufs[bufno].str, inet_ntoa(addr), sizeof(bufs[bufno].str));
    s = bufs[bufno].str;
    bufno = (bufno+1) % NUM_BUFS;
    return s;
#undef NUM_BUFS
}

It introduces 4 different temporary buffers to wrap inet_ntoa function since inet_ntoa is not re-entrant.
But seems to me this naddr_ntoa function is also not re-entrant:
the static bufno variable can be manipulated by other so the temporary buffers do not seem work as expected here.

So is it a potential bug?

Actually, it seems to me they wrote it this way to avoid `malloc`, rather than for re-entrancy reasons. `inet_ntoa`, according to at least one man page I read, uses only one static buffer (which might have been deemed unacceptable to the NetBSD developers). — nneonneo, Sep 20 '12 at 06:10

Some programmer dude · Answer 1 · 2012-09-20T07:01:29.453

2

Yes, this is a potential bug. If you want a similar function that most likely reentrant you could use e.g. inet_ntop (which incidentally handles IPv6 as well).

edited Sep 20 '12 at 07:01

answered Sep 20 '12 at 06:11

Some programmer dude

400,186
35
402
621

Can you add a citation for its reentrancy? – nneonneo Sep 20 '12 at 06:12
1

@nneonneo Well, you supply your own buffer instead if having a global/static buffer hidden somewhere, ergo it's reentrant (unless it's _called_ in a non-reentrant way of course). – Some programmer dude Sep 20 '12 at 06:15
Playing the devil's advocate here but it could have other internal state making it not re-entrant. (Not that I believe it has.) – Prof. Falken Sep 20 '12 at 06:23
`printf` isn't reentrant, but your logic would suggest it is. What if `inet_ntop` calls `sprintf` (which is not guaranteed to be reentrant)? – nneonneo Sep 20 '12 at 06:51
@nneonneo You are correct. Everywhere I look everyone _says_ it's reentrant, but reading manual pages (including the POSIX one linked in my answer) it says _nothing_ about reentrancy or thread safety. I'll rephrase my answer. :) – Some programmer dude Sep 20 '12 at 07:00
Don't you just hate how reentrancy is an afterthought in most of POSIX? – nneonneo Sep 20 '12 at 07:13

score 0 · Answer 2 · answered Nov 11 '12 at 02:14

That code comes from src/sbin/routed/trace.c and it is not a general library routine, but just a custom hack used only in the routed program. The addrname() function in the same file makes use of the same trick, for the same reason. It's not even NetBSD code per se, but rather it comes from SGI originally, and is maintained by Vernon Schryver (see The Routed Page).

It's just a quick hack to allow use of multiple calls within the same expression, such as where the results are being used in one printf() call: E.g.:

printf("addr1->%s, addr2->%s, addr3->%s, addr4->%s\n",
       naddr_ntoa(addr1), naddr_ntoa(addr2), naddr_ntoa(addr3), naddr_ntoa(addr4));

There are several examples of similar uses in the routed source files (if.c, input.c, rdisc.c).

There is no bug in this code. The routed program is not multi-threaded. Reentrancy is not being addressed at all in this hack. This trick has been done by design for a very specific purpose that has nothing to do with reentrancy. The Code Reading author(s) is wrong to associate this trick with reentrancy.

It's simply a way to hide the saving of multiple results in an array of static variables instead of having to individually copy those results from one static variable into separate storage in the calling function when multiple results are required for a single expression.

Remember that static variables have all the properties of global variables except for the limited scope of their identifier. It is of course true that unprotected use of global (or static) variables inside a function make that function non-reentrant, but that's not the only problem global variables cause. Use of a fully-reentrant function would not be appropriate in routed because it would actually make the code more complex than necessary, whereas this hack keeps the calling code clean and simple. It would though have been better for the hack to be properly documented such that future maintainers would more easily spot when NUM_BUFS has to be adjusted.

Reentrancy or not with this netbsd code

2 Answers2