Bug 11864 - Use of non-portable clearenv() function
Summary: Use of non-portable clearenv() function
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build (show other bugs)
Version: 4.3.8
Hardware: All FreeBSD
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-21 00:23 UTC by Timur Bakeyev
Modified: 2016-06-02 10:19 UTC (History)
3 users (show)

See Also:


Attachments
Replace clearenv with direct manipulation of environ (2.08 KB, patch)
2016-04-27 18:23 UTC, Jérémie Courrèges-Anglas
no flags Details
patch for 4.4 (3.87 KB, patch)
2016-05-19 12:44 UTC, Volker Lendecke
gd: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timur Bakeyev 2016-04-21 00:23:02 UTC
On FreeBSD 10.3 samba 4.3.8 build fails with:

../source3/client/smbspool_krb5_wrapper.c:198:2: error: implicit declaration of function 'clearenv' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        clearenv();
        ^
../source3/client/smbspool_krb5_wrapper.c:198:2: note: did you mean 'clearerr'?
/usr/include/stdio.h:237:7: note: 'clearerr' declared here
void     clearerr(FILE *);
         ^
Well, compiler trues to be too smart, but basically it points out that clearenv() function doesn't exist on FreeBSD.

Manpage for clearenv() says:

DESCRIPTION
       The clearenv() function clears the environment of all name-value pairs and sets the value of the external variable environ to NULL.

RETURN VALUE
       The clearenv() function returns zero on success, and a nonzero value on failure.

VERSIONS
       Not in libc4, libc5.  In glibc since glibc 2.0.

CONFORMING TO
       Various  UNIX  variants  (DG/UX, HP-UX, QNX, ...).  POSIX.9 (bindings for FORTRAN77).  POSIX.1-1996 did not accept clearenv() and putenv(3), but changed its
       mind and scheduled these functions for some later issue of this standard (cf. B.4.6.1).  However, POSIX.1-2001 only adds putenv(3), and rejected clearenv().

NOTES
       Used in security-conscious applications.  If it is unavailable the assignment

           environ = NULL;

       will probably do.

       The DG/UX and Tru64 man pages write: If environ has been modified by anything other than the putenv(3), getenv(3), or clearenv() functions, then  clearenv()
       will return an error and the process environment will remain unchanged.


Which again points that it's not fully portable.

Please, consider either proper test for it in the WAF, or add it to the libreplace or just replace with environ = NULL;
Comment 1 Jérémie Courrèges-Anglas 2016-04-27 18:22:38 UTC
This also affects OpenBSD. To work around this I currently use something akin to the attached patch (against master).  A rationale for using calloc can be found in the commit message.

But instead of clearing the environment, adding a few selected environment variables and then calling exec*(), one could just use execve(2) which allows passing an environment parameter. execve(2) is specified by POSIX.  If there is interest I can cook such a patch for smbspool_krb5_wrapper.
Comment 2 Jérémie Courrèges-Anglas 2016-04-27 18:23:34 UTC
Created attachment 12030 [details]
Replace clearenv with direct manipulation of environ
Comment 3 Jérémie Courrèges-Anglas 2016-04-27 18:24:28 UTC
Oh and, it also affects 4.4.2.
Comment 4 Jeremy Allison 2016-04-28 00:55:21 UTC
(In reply to Jérémie Courrèges-Anglas from comment #2)

That patch looks fine to me. Can you post it to samba-technical so it can get 2+ team member review and get integrated to master and then back-ported ?

Cheers,

Jeremy.
Comment 5 Jérémie Courrèges-Anglas 2016-04-28 16:11:39 UTC
(In reply to Jeremy Allison from comment #4)
Looks like Volker beat me to it!
Comment 6 Jérémie Courrèges-Anglas 2016-05-19 12:01:23 UTC
Any chance to see this land in v4-4?  The commits in master are f198abc and  e0d8c6b.  I can prepare a diff on v4-4-test, if deemed useful.
Comment 7 Volker Lendecke 2016-05-19 12:44:23 UTC
Created attachment 12117 [details]
patch for 4.4
Comment 8 Guenther Deschner 2016-05-20 15:07:37 UTC
Comment on attachment 12117 [details]
patch for 4.4

LGTM
Comment 9 Guenther Deschner 2016-05-20 15:08:21 UTC
Karolin, please add to 4.4.x.

Thanks!
Comment 10 Jérémie Courrèges-Anglas 2016-05-20 15:51:33 UTC
Thanks.  Note that Timur Bakeyev reported this problem on samba-4.3.8, maybe a backport to v4-3-test could be useful too.
Comment 11 Karolin Seeger 2016-05-30 08:45:38 UTC
(In reply to Guenther Deschner from comment #9)
Pushed to autobuild-v4-4-test.
Comment 12 Karolin Seeger 2016-05-30 08:47:02 UTC
(In reply to Jérémie Courrèges-Anglas from comment #10)
Patches apply cleanly on current v4-3-test branch.
Could someone please ack the patches for 4.3, please?

Thanks!
Comment 13 Karolin Seeger 2016-06-01 07:32:19 UTC
Günther, Jeremy, Volker, if someone would comment that the patch is correct for 4.3 also (applies cleanly), it can be included in the upcoming 4.3 maintenance release.

Thanks!
Comment 14 Karolin Seeger 2016-06-01 07:33:24 UTC
(In reply to Karolin Seeger from comment #11)
Pushed to v4-4-test.
Comment 15 Volker Lendecke 2016-06-01 09:31:27 UTC
(In reply to Karolin Seeger from comment #13)
> Günther, Jeremy, Volker, if someone would comment that the patch is correct
> for 4.3 also (applies cleanly), it can be included in the upcoming 4.3
> maintenance release.

Yes, the patch makes sense in 4.3 too.

Volker
Comment 16 Karolin Seeger 2016-06-01 09:39:38 UTC
(In reply to Volker Lendecke from comment #15)
Thanks, Volker!
Comment 17 Karolin Seeger 2016-06-01 09:40:31 UTC
Pushed to autobuild-v4-3-test.
Comment 18 Karolin Seeger 2016-06-02 10:19:59 UTC
(In reply to Karolin Seeger from comment #17)
Pushed to v4-3-test.
Closing out bug report.

Thanks!