magic.c should not close random file fd.

[ehoffman2]

In magic.c (the file that generate random numbers), the first time the function magic() is called, magic is initialized. Upon initialization, magic_inited is set (a global static), and rfd (the file descriptor for urandom) is also set.

Then, in main magic() function, a randum number is read from rfd, followed immediately by a close() of rfd.

[...]
magic_init();

if(rfd > -1) {
    u_int32_t ret;
    int nb_read = read(rfd, &ret, sizeof(ret));
    close(rfd);

[...]

This cause error on subsequent calls. In my case it cause the side effect of hanging the library because that file descriptor is re-assigned to the TACACS network socket (accounting hang). Accounting try to get another random number, and hang reading the network socket instead of urandom.

rfd must not be closed. It is a one-time statically initialized variable.

This bug was just lately introduced (Nov 26, 2014).

Regards,
Eric

[benschumacher]

Eric-

The previous issue #23 indicates that leaving the file open causes problems in some scenarios. I think closing the FD is an appropriate solution, but it seems like this could be a race condition and is possible related to being run multithreaded -- the code not being particularly robust for this sort of behavior. (NSCD, for example, is threaded.)

If you changed the magic() function to this definition, I believe it would solve this issue, if indeed it is the issue (you might need to #include <pthread.h>, too):

#include <pthread.h>
/*
 * magic - Returns the next magic number.
 */
u_int32_t
magic()
{
    static pthread_once_t magic_control = PTHREAD_ONCE_INIT;

    pthread_once(&magic_control, &magic_init);

    return (u_int32_t)random();
}

(I haven't tried running this code yet, but I have confirmed it does compile.)

I can also create a pull request for it when I get a chance. With this change, the 'magic_initialised' variable isn't used any longer, so it could be removed, as well.

Hope this helps,
Ben

[daveolson53]

benschumacher notifications@github.com wrote:

The previous issue #23 indicates that leaving the file open causes problems in
some scenarios. I think closing the FD is an appropriate solution, but it seems
like this could be a race condition and is possible related to being run
multithreaded -- the code not being particularly robust for this sort of
behavior. (NSCD, for example, is threaded.)

If you changed the magic() function to this definition, I believe it would
solve this issue, if indeed it is the issue (you might need to #include
<pthread.h>, too):

I think that's not a good idea. Big chunks of the code aren't thread
safe, so there isn't a reason to patch this one tiny thing.

I'd suggest saving that until the code is made thread safe, and do all the work
together.

Dave Olson
olson@cumulusnetworks.com

[ehoffman2]

I don't think it's multi-thread related. In my scenario, I have only one thread running the code. The problem really is because when the function is called the first time, the file is opened (rfd is initialized) and is then closed (thus, the referenced FD is obsolete) before return to caller. The second time magic() is called (within the same tread), the file descriptor is now invalid (and not re-initialized). Data is read from whatever file is referenced by the obsolete descriptor.

Regards,
Eric

[daveolson53]

ehoffman2 notifications@github.com wrote:

I don't think it's multi-thread related. In my scenario, I have only one thread
running the code. The problem really is because when the function is called the
first time, the file is opened (rfd is initialized) and is then closed (thus,
the referenced FD is obsolete) before return to caller. The second time magic()
is called (within the same tread), the file descriptor is now invalid (and not
re-initialized). Data is read from whatever file is referenced by the obsolete
descriptor.

That sounds like the problem that was addressed by an earlier change.
Maybe I'm confused, though.

I any case, I stand by my recommendation that we not try to apply
the thread patch.

Dave Olson
olson@cumulusnetworks.com

[kravietz]

The whole magic.c is now replaced by OpenSSL.