getpass regressions on Solaris/Illumos - 3.6 and master.

[Jelmer Vernooij]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/29/2012 10:48 AM, Andrew Bartlett wrote:
> On Sun, 2012-01-29 at 00:24 -0500, Ira Cooper wrote:
>> There's been at least 2 regressions on the Solaris platforms. One
>> involving net join not working, and one involving smbclient not working,
>> using typed passwords.
>>
>> I ended up tracking this issue down via bisection
>> to: 510e61871c0c2b2659b80d5f785522184131b1d9 and then working out why that
>> patch was the issue.
>>
>> If you look at the commit involved, it looks like a no-op. Alas, util.c
>> included "system/passwd.h" and util_cmdline.c did not, so there was a
>> regression, because getpass doesn't take passwords longer than 8
>> characters, on Solaris.
>>
>> I'm looking for a better idea than just adding "system/passwd.h" to
>> source3/include/includes.h, that also has the effect of protecting against
>> the kind of simple mistake made in the commit above. I can't blame the
>> committer for the error, it was far too easy to make, in my opinion.
>>
>> Patch enclosed, and verified to work, against v3-6-test, that just adds
>> "system/passwd.h" to source3/include/includes.h.
>>
>> Thanks for the explanation of the brokenness of getpass on Solaris, Andrew
>> Bartlett.
>
> I think the correct fix is change the getpass() calls to be explicit
> calls directly to the Samba replacement functions (which would be
> renamed and moved to samba-util).
>
> We should then only fall back if we find a real-world system where the
> replacement (now samba-util) function fails to compile, that we wish to
> support, and it has a non-broken getpass()/getpassphrase().
>
> (This is the opposite to our normal practice with replacement functions,
> and I queried this myself in years past, but it was put to me
> persuasively that otherwise the replacement will never be tested).
>
> This will greatly reduce the chance that this might happen again.
The defines for using the alternative functions should be in replace.h,
so we can't miss them if we happen to forget one of the system/ includes.

The reason I added libreplace as a separate thing from the code in lib/
in the first place was exactly to be able to avoid this. Libreplace
should be as transparent as possible, so we can just do the right thing
on sane platforms, and it will just work on other platforms. We
shouldn't have to worry about calling wrapper functions in case there
are platforms on which a function is broken.

Originally it was also possible to compile with just an empty replace.h
and without having libreplace at all on modern Linux systems.
Unfortunately that's no longer possible because of the system/ headers.

Cheers,

Jelmer
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJPJo+dAAoJEACAbyvXKaRXRBsP/RdKocB0LjyirzFiesAZiVp5
mYZ8VK0Hw5umW3kcI86Sh0/GxedY84bDCCITXX6Rn2jcfVEND9HiRxroRrR/HayM
D5tYBZLJIdKqMM7wX12tJa0v6uKMr9aiCv8g+tMBQV0hZH2EzWRVU9dFMDRRkFD/
VFOhfSCIDohZO/dvCNdwBWz6aRzpllyB9PEfVbuv6JBLy/VQWnXHrOXqt9+gWQcc
Ls2LX5pNvmKVYoCTOLO3YRUGN69TeakVYRL1CX/gdKo8l9SKgca9/L0Cu7ul0lcw
3iQyO0WnKDD2zZnmMjXQEWfq6mAVgcgtbKUz+0XEorh+hqeam4+Z8P/QC2Jj+owE
TAQNkwpRT3N/m7Wocmi9I+cMWc9fDbSzM8QO1p5rff6niVMF6yYXWpuPCx/OWpK/
fDFghD0F0fH2Tuk49YMKIv7auCXaQeLdWMYwvQ1hMgbc8+XSXmMoOe07c2ysKdOf
CrXCzPOOIySKMfH9HJsF3TVlihABBudynFAgDAAW7CK9AuNZYOPuLW3/v3Kb1Bik
EuERKrlBQEgp7LIMXNnRD4SUTv6vs+znrmLJlxpJWy8F2OtZRxTr9fDfDyS7NRQH
E/JdEnZO0dKhyEFcS2I9oAnRX4Dbz4IdnWLBN1MORK+HWyPYAA1R93HoOqN6las9
YSL3QR2BWVswS+KJbSu/
=xbac
-----END PGP SIGNATURE-----