Bug 4109 - smbd fails to restore WINBINDD_DONT_ENV env. var. and fails to communicate with windbindd
smbd fails to restore WINBINDD_DONT_ENV env. var. and fails to communicate wi...
Status: RESOLVED FIXED
Product: Samba 3.0
Classification: Unclassified
Component: winbind
3.0.23c
x86 FreeBSD
: P3 major
: none
Assigned To: Gerald (Jerry) Carter
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-18 11:50 UTC by Andriy Gapon
Modified: 2006-10-19 17:39 UTC (History)
1 user (show)

See Also:


Attachments
Work around getenv/putenv async (2.56 KB, patch)
2006-09-23 21:06 UTC, Timur Bakeyev
no flags Details
Updated patch (2.94 KB, patch)
2006-09-25 05:24 UTC, Timur Bakeyev
no flags Details
one more attempt (2.96 KB, patch)
2006-09-25 06:34 UTC, Timur Bakeyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andriy Gapon 2006-09-18 11:50:16 UTC
This is not a winbindd bug, but a winbind-related one.

In two places the following approach for temporary disabling winbind communication is used (pseudo code):

char* env = getenv(...);
winbind_off(); //{ putenv(WINBINDD_DONT_ENV=1) }
.
.
.
if (!(env && strequal(env, "1")) {
      winbind_on(); //{ putenv(INBINDD_DONT_ENV=0) }
}

This approach is not safe and won't work on some systems, because variable env is a *pointer* into environment space of a program, not a value or a copy. Thus, it is possible that setenv(3)/putenv(3) would simply overwrite a value at the same location, e.g.:
env = getenv(ENV_VAR);
printf("%s\n", env); //prints old val
putenv(ENV_VAR=newval);
printf("%s\n", env); //same pointer, but prints new val

If this is the case, after winbind_off() env would point to string "1" and thus the quoted pseudo-code would fail to restore original state/value of that variable.

On FreeBSD 6.1 I see the follwing behavior that only after the very first winbind_off() the code calls winbind_on() (this happens because env was originally NULL, i.e. env.var. was undet), after the second winbind_off() the code never calls winbind_on(), because of teh reasons explained above.

I think that the easiest way in this situtation is to store *value* of environment variable somewhere, e.g.:

char* env = getenv(WINBINDD_DONT_ENV);
char winbind_disabled_flag = env ? env[0] : '0';
winbind_off();
...
if (winbind_disabled_flag == '0')
     winbind_on();
Comment 1 Andriy Gapon 2006-09-19 07:34:07 UTC
I submitted this bug report in a haste, so I would like add some description of the symptoms and system:

FreeBSD 6.1-RELEASE-p2 i386
samba-3.0.23c,1 built from sources via the FreeBSD port

Samba is configured as a work-group member (not a domain controller or anything) with security=domain and uses winbind to get user/group information from Windows domain controller (and also has some shares with restricted access via "valid users" directive of form valid users = @DOMAIN.group).

I can verify that configuration of winbindd, nsswitch and samba is OK:
all wbinfo commands work well
system command 'id' works well for (non-local) groups and users in my windows workgroup

On the other hand, any access attempt to the samba server in question resulted in NT_STATUS_NO_LOGON_SERVERS errors.

My debugging showed that this happend because winbindd_request_response() was actually a NOP, so response was not touched at all and remain zeroed-out structure and thus both response.data.auth.nt_status and response.extra_data.data were both zeros/nulls.

My first post explained why winbindd_request_response() was a NOP -- environment variable defined in WINBINDD_DONT_ENV macro was set to 1 and never reset back to 0 due to unsafe usage of getenv/putenv and peculiarities of the particular FreeBSD libc with respect to those functions.
Comment 2 Timur Bakeyev 2006-09-23 18:38:19 UTC
(In reply to comment #1)

Hi Andriy!

Thanks a lot for your perfect analysis of the problem! I got few reports about non-working winbindd in FreeBSD, but couldn't imagine, that the problem was in inproper usage of getenv/putenv.

I'll try to make patch for the port meanwhile and, lets hope, Samba team will come with more permanent solution for the problem.

With best regards,
Timur.
Comment 3 Timur Bakeyev 2006-09-23 21:06:49 UTC
Created attachment 2151 [details]
Work around getenv/putenv async
Comment 4 Timur Bakeyev 2006-09-23 21:08:59 UTC
(In reply to comment #1)

Hi, Andryi!

Can you please try attached patch? Hope, it fixes the problem, you encountered.

If so, I'll add it to net/samba3 ASAP.

With best regards,
Tmur
Comment 5 Tim Aslat 2006-09-25 01:27:24 UTC
(In reply to comment #4)

Have tried the patch, without success.  Still getting 
NT_STATUS_NO_LOGON_SERVERS error 

wbinfo all commands work
shares not available (NT_STATUS_NO_LOGON_SERVERS error)
users available via pam_winbind

unless I'm doing something wrong
Comment 6 Andriy Gapon 2006-09-25 04:10:23 UTC
(In reply to comment #4)

Timur, I haven't tried your patch but I see that it won't work - you need to *save* state of environment *before* calling winbind_off(), because winbind_off() modifies the environment and thus winbind_env() will always return True when called after winbind_off().


Comment 7 Timur Bakeyev 2006-09-25 04:58:58 UTC
(In reply to comment #6)
> (In reply to comment #4)
> 
> Timur, I haven't tried your patch but I see that it won't work - you need to
> *save* state of environment *before* calling winbind_off(), because
> winbind_off() modifies the environment and thus winbind_env() will always
> return True when called after winbind_off().

Yeh, shouldn't do such things at 5a.m. in the morning :)

Wait a bit, I'll send a new version :)

Cheers,
Timur.
Comment 8 Timur Bakeyev 2006-09-25 05:24:04 UTC
Created attachment 2158 [details]
Updated patch

Hopefuly, this one is ok :)
Comment 9 Timur Bakeyev 2006-09-25 05:26:23 UTC
(In reply to comment #6)
> (In reply to comment #4)

Hi, Andryi!

Can you try a new patch? Hope, it's ok.
Comment 10 Timur Bakeyev 2006-09-25 05:28:06 UTC
(In reply to comment #5)
> (In reply to comment #4)

Hi, Tim!

Can you try new patch? I made a mistake in previous one, so it wasn't working. Thanks to Andryi for spotting it.

With regards,
Timur.
Comment 11 Timur Bakeyev 2006-09-25 06:34:37 UTC
Created attachment 2160 [details]
one more attempt

Name of the variable and function clushed :> So, did some renaming.
Comment 12 Tim Aslat 2006-09-25 09:18:35 UTC
(In reply to comment #10)
> Hi, Tim!
> 
> Can you try new patch? I made a mistake in previous one, so it wasn't working.
> Thanks to Andryi for spotting it.


I couldn't make it work, hopefully Andryi has better luck than I did
Comment 13 Andriy Gapon 2006-09-25 10:26:32 UTC
(In reply to comment #11)
Yes, this patch fixes the problem for me! Thank you!
Not sure about the name of the new function :-) but important thing is that it does the job.
Comment 14 Gerald (Jerry) Carter 2006-09-26 08:11:58 UTC
Timur,  The patch initially looks right.  I'll try to finish reviewing 
it today.
Comment 15 Denis Eremenko 2006-09-28 22:24:35 UTC
This patch solved the problem. Thanks! FreeBSD 6.2-PRERELEASE i386
Comment 16 Timur Bakeyev 2006-10-01 21:01:19 UTC
(In reply to comment #15)
> This patch solved the problem. Thanks! FreeBSD 6.2-PRERELEASE i386

Thanks, Denis, for the feedback! I'd try to commit it into the port before the freeze.

With best regards,
Timur.

Comment 17 Timur Bakeyev 2006-10-10 20:24:08 UTC
Hi, Jerry!

(In reply to comment #14)
> Timur,  The patch initially looks right.  I'll try to finish reviewing 
> it today.

One more problem with this patch showed up today - it affects pam_winbind.so (and, possibly, nss_windbind.so) due usage of strequal() function for comparation with "1". It defined in util_str.c, which is not linked against pam_winbind.so.

The solution is either to add all bunch of files, that implement strequal() and dependencies or simply use strcmp(), which comes with libc and safe to use in this case.

Sorry for overlooking this problem :(

With best regards,
Timur.

Comment 18 Andriy Gapon 2006-10-11 06:09:40 UTC
or to simply use comparisons for char array elements, like (pseudo-code):
char[0]=='1' && char[1]=='\0'
Comment 19 Gerald (Jerry) Carter 2006-10-19 17:39:58 UTC
Looks good. Applying to SAMBA_3_0 and SAMBA_3_0_23