magic.c should not close random file fd. about pam_tacplus HOT 5 CLOSED

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

from pam_tacplus.

benschumacher [email protected] 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
[email protected]

from pam_tacplus.

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

from pam_tacplus.

ehoffman2 [email protected] 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
[email protected]

from pam_tacplus.

The whole magic.c is now replaced by OpenSSL.

from pam_tacplus.