Bug 1566464 - glibc: Consider declaring crypt in <unistd.h>, as before
Summary: glibc: Consider declaring crypt in <unistd.h>, as before
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: glibc
Version: 28
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Florian Weimer
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard: RejectedBlocker;RejectedFreezeException
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-04-12 11:19 UTC by Nikos Mavrogiannopoulos
Modified: 2019-04-02 11:43 UTC (History)
14 users (show)

Fixed In Version: glibc-2.27.9000-30.fc29 glibc-2.27-21.fc28
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-04-02 11:43:26 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Reproducer (656 bytes, text/plain)
2018-04-12 11:19 UTC, Nikos Mavrogiannopoulos
no flags Details

Description Nikos Mavrogiannopoulos 2018-04-12 11:19:15 UTC
Description of problem:
When compiling code under fedora28 which works under 27, I see the following issues.
1. The definition of crypt() is not available if _XOPEN_SOURCE is defined in unistd.h

2. The code crashes

How reproducible:
Compile the attached code under F27 with -lcrypt. Output (under valgrind -q):
passwd: $5$123456789abcdefg$lnJxX23qD4ZYtBIRkeV675g9wQ4YYTgPiFP4DDfDGl5

Under F28:
Segmentation fault

Comment 1 Nikos Mavrogiannopoulos 2018-04-12 11:19:47 UTC
Created attachment 1420812 [details]
Reproducer

Comment 2 Nikos Mavrogiannopoulos 2018-04-12 11:20:35 UTC
The output when compiling in F28 is:
```
$ gcc c.c -lcrypt
c.c: In function ‘main’:
c.c:38:25: warning: implicit declaration of function ‘crypt’; did you mean ‘chroot’? [-Wimplicit-function-declaration]
  printf("passwd: %s\n", crypt(passwd, salt));
                         ^~~~~
                         chroot

```

Comment 3 Nikos Mavrogiannopoulos 2018-04-12 11:24:37 UTC
Note, that if I include <crypt.h> instead of <unistd.h> with XOPEN_SOURCE, everything works under F28.

I did not include crypt.h, because both F27 and F28 manpages for crypt as to include unistd.h with _XOPEN_SOURCE defined.

Comment 4 Florian Weimer 2018-04-12 11:37:44 UTC
You should really build with -Werror=implicit-function-declaration or use C++.  It's a GCC bug that it doesn't reject implicit function declarations; they were removed in C99.  We can't change the default because too many configure checks would fail to detect feature support.

With the current libxcrypt transition approach, it is expected that <unistd.h> no longer contains a declaration of crypt.  (It's an optional POSIX feature.)

We need to upstream our patch and see how this is resolved.

Comment 5 Fedora Blocker Bugs Application 2018-04-12 11:43:08 UTC
Proposed as a Blocker and Freeze Exception for 28-final by Fedora user nmav using the blocker tracking app because:

 This issue makes well-behaving software which use crypt() in F27 to crash in F28 after recompilation.

Comment 6 Nikos Mavrogiannopoulos 2018-04-12 11:48:41 UTC
> You should really build with -Werror=implicit-function-declaration or use C++.  

Well, I'm using C, and crypt() is a function for C applications :)
In my view, the declaration not being provided by glibc is the issue, not that the application is compiled with the wrong gcc flags. In fedora27 the same application compiles with _no warnings_.

> With the current libxcrypt transition approach, it is expected that <unistd.h> no longer contains a declaration of crypt.  (It's an optional POSIX feature.)

This is not documented in the manpages (manpage of crypt is the same in F28 and F27), and even if we address that no transition period was made available.

Comment 7 Carlos O'Donell 2018-04-12 14:19:33 UTC
(In reply to Nikos Mavrogiannopoulos from comment #6)
> > You should really build with -Werror=implicit-function-declaration or use C++.  
> 
> Well, I'm using C, and crypt() is a function for C applications :)

You have to limit this to the standards you want to support and which option groups in the given standards.

For example ISO C does not define crypt().

> In my view, the declaration not being provided by glibc is the issue, not
> that the application is compiled with the wrong gcc flags. In fedora27 the
> same application compiles with _no warnings_.

We guarantee ABI compatibility not API compatibility, you may always need to change your code if you recompile your application.
 
> > With the current libxcrypt transition approach, it is expected that <unistd.h> no longer contains a declaration of crypt.  (It's an optional POSIX feature.)
> 
> This is not documented in the manpages (manpage of crypt is the same in F28
> and F27), and even if we address that no transition period was made
> available.

We no longer provide the optional XSI feature defining crypt in unistd.h.

That is to say that _XOPEN_CRYPT is no longer supported in glibc, we are migrating to libxcrypt.

The man pages are directly from the man pages project, and we need to update those.

Florian,

Must Fedora glibc define _XOPEN_CRYPT to -1? This indicates the option group is disabled in glibc (no support for crypt(), encrypt() and setkey()), and would comply with XSI Option Group rules for disclaiming conformance.

Then we could add some notes to the linux kernel man pages about what to do if _XOPEN_CRYPT was set to -1 (see XSI Option Groups in POSIX Issue 7).

Note that on Rawhide the compiler *does* issue a warning and prevents compilation.

Comment 8 Florian Weimer 2018-04-12 14:39:25 UTC
(In reply to Carlos O'Donell from comment #7)
> Must Fedora glibc define _XOPEN_CRYPT to -1? This indicates the option group
> is disabled in glibc (no support for crypt(), encrypt() and setkey()), and
> would comply with XSI Option Group rules for disclaiming conformance.

I think POSIX says that an undefined macro is equivalent to -1, and it's already undefined in Fedora, so this part at least is consistent.

We should follow upstream in this area.  If we can't upstream the patch, I'm not sure what we can do here.

Comment 9 Carlos O'Donell 2018-04-12 14:45:42 UTC
(In reply to Florian Weimer from comment #8)
> (In reply to Carlos O'Donell from comment #7)
> > Must Fedora glibc define _XOPEN_CRYPT to -1? This indicates the option group
> > is disabled in glibc (no support for crypt(), encrypt() and setkey()), and
> > would comply with XSI Option Group rules for disclaiming conformance.
> 
> I think POSIX says that an undefined macro is equivalent to -1, and it's
> already undefined in Fedora, so this part at least is consistent.
> 
> We should follow upstream in this area.  If we can't upstream the patch, I'm
> not sure what we can do here.

OK, as co-owner of the system-wide change (https://fedoraproject.org/wiki/Changes/Replace_glibc_libcrypt_with_libxcrypt) I expect you will follow-up in upstream?

I note the POSIX language for XSI Option Groups says:
~~~
An implementation that claims conformance to this Option Group shall set _XOPEN_CRYPT to a value other than -1.
~~~
So it's not clear that we are allowed to undefine _XOPEN_CRYPT as equivalent to -1.

I can help write the linux man pages update once we conclude this, as an independent reviewer.

Comment 10 Florian Weimer 2018-04-12 14:50:00 UTC
(In reply to Carlos O'Donell from comment #9)
> (In reply to Florian Weimer from comment #8)
> > (In reply to Carlos O'Donell from comment #7)
> > > Must Fedora glibc define _XOPEN_CRYPT to -1? This indicates the option group
> > > is disabled in glibc (no support for crypt(), encrypt() and setkey()), and
> > > would comply with XSI Option Group rules for disclaiming conformance.
> > 
> > I think POSIX says that an undefined macro is equivalent to -1, and it's
> > already undefined in Fedora, so this part at least is consistent.
> > 
> > We should follow upstream in this area.  If we can't upstream the patch, I'm
> > not sure what we can do here.
> 
> OK, as co-owner of the system-wide change
> (https://fedoraproject.org/wiki/Changes/
> Replace_glibc_libcrypt_with_libxcrypt) I expect you will follow-up in
> upstream?
> 
> I note the POSIX language for XSI Option Groups says:
> ~~~
> An implementation that claims conformance to this Option Group shall set
> _XOPEN_CRYPT to a value other than -1.
> ~~~
> So it's not clear that we are allowed to undefine _XOPEN_CRYPT as equivalent
> to -1.

There is also this:

“The implementation advertises at compile time (by defining the constant in <unistd.h> with value -1, or by leaving it undefined) that the option is not supported for compilation and, at the time of compilation, is not supported for runtime use.”

So I think we are covered.

Comment 11 Carlos O'Donell 2018-04-12 19:44:48 UTC
(In reply to Florian Weimer from comment #10)
> (In reply to Carlos O'Donell from comment #9)
> > (In reply to Florian Weimer from comment #8)
> > > (In reply to Carlos O'Donell from comment #7)
> > > > Must Fedora glibc define _XOPEN_CRYPT to -1? This indicates the option group
> > > > is disabled in glibc (no support for crypt(), encrypt() and setkey()), and
> > > > would comply with XSI Option Group rules for disclaiming conformance.
> > > 
> > > I think POSIX says that an undefined macro is equivalent to -1, and it's
> > > already undefined in Fedora, so this part at least is consistent.
> > > 
> > > We should follow upstream in this area.  If we can't upstream the patch, I'm
> > > not sure what we can do here.
> > 
> > OK, as co-owner of the system-wide change
> > (https://fedoraproject.org/wiki/Changes/
> > Replace_glibc_libcrypt_with_libxcrypt) I expect you will follow-up in
> > upstream?
> > 
> > I note the POSIX language for XSI Option Groups says:
> > ~~~
> > An implementation that claims conformance to this Option Group shall set
> > _XOPEN_CRYPT to a value other than -1.
> > ~~~
> > So it's not clear that we are allowed to undefine _XOPEN_CRYPT as equivalent
> > to -1.
> 
> There is also this:
> 
> “The implementation advertises at compile time (by defining the constant in
> <unistd.h> with value -1, or by leaving it undefined) that the option is not
> supported for compilation and, at the time of compilation, is not supported
> for runtime use.”
> 
> So I think we are covered.

I confirm this, and verified this works on F27, where sysconf _SC_XOPEN_CRYPT returns 1, while in F28/F29 it returns -1, indicating glibc is not supporting crypt anymore.

Posted two patches upstream with linux man pages project to clarify the behaviour for any C library not implementing _XOPEN_ENCRYPT and switching to libxcrypt:
https://marc.info/?l=linux-man&m=152355993831235&w=2
https://marc.info/?l=linux-man&m=152355994531241&w=2

Once that goes into upstream master, then we can flip this bug to man-pages project to get a backport of the changes.

Comment 12 Adam Williamson 2018-04-12 21:20:52 UTC
Florian: Carlos: do either of you see anything we should block the F28 release on, here?

Comment 13 Carlos O'Donell 2018-04-12 21:39:22 UTC
(In reply to Adam Williamson from comment #12)
> Florian: Carlos: do either of you see anything we should block the F28
> release on, here?

No.

I do not see any blocker. 

The enhanced documentation (see comment #11) could proceed in parallel and software has to be updated to include crypt.h. In glibc we never promise source-level compatibility, only that existing binaries will continue to run. Once you need to recompile you *may* have to adjust things slightly. This is just such a case as we implement the libxcrypt system-wide migration (https://fedoraproject.org/wiki/Changes/Replace_glibc_libcrypt_with_libxcrypt).

Comment 14 Nikos Mavrogiannopoulos 2018-04-13 06:11:16 UTC
I think the upgrade/compatibility impact of the change needs update:
Existing software following the suggestions of the crypt(3) manpage will not work under Fedora 28. The software may compile, but crash at run-time. Application developers and maintainers should ensure that the <crypt.h> header is included instead of the recommended <unistd.h> by the manpage.

Comment 15 Nikos Mavrogiannopoulos 2018-04-13 06:35:45 UTC
I've checked the patches for the man-pages, and I do not see any change in the headers listed. Did you miss the following?

diff --git a/man3/crypt.3 b/man3/crypt.3
index 58c62d93d..8969ba0e7 100644
--- a/man3/crypt.3
+++ b/man3/crypt.3
@@ -38,13 +38,9 @@
 crypt, crypt_r \- password and data encryption
 .SH SYNOPSIS
 .nf
-.BR "#define _XOPEN_SOURCE" "       /* See feature_test_macros(7) */"
-.B #include <unistd.h>
+.B #include <crypt.h>
 .PP
 .BI "char *crypt(const char *" key ", const char *" salt );
-
-.BR "#define _GNU_SOURCE" "         /* See feature_test_macros(7) */"
-.B #include <crypt.h>
 .PP
 .BI "char *crypt_r(const char *" key ", const char *" salt ,
 .BI "              struct crypt_data *" data );

Comment 16 Nikos Mavrogiannopoulos 2018-04-13 06:37:35 UTC
There is also a typo with _XOPEN_CRPYT

Comment 17 Carlos O'Donell 2018-04-13 15:14:36 UTC
(In reply to Nikos Mavrogiannopoulos from comment #14)
> I think the upgrade/compatibility impact of the change needs update:
> Existing software following the suggestions of the crypt(3) manpage will not
> work under Fedora 28. The software may compile, but crash at run-time.
> Application developers and maintainers should ensure that the <crypt.h>
> header is included instead of the recommended <unistd.h> by the manpage.

Sounds good to me.

Florian, What do you think?

(In reply to Nikos Mavrogiannopoulos from comment #15)
> I've checked the patches for the man-pages, and I do not see any change in
> the headers listed. Did you miss the following?

No. Because it's not true upstream.

The only comment I could make upstream was this:
~~~
Availability in glibc

The crypt (), encrypt (), and setkey () functions are part of the POSIX.1-2008 XSI Options Group for Encryption and are optional. If the interfaces are not available then the symbolic constant _XOPEN_CRYPT is either not defined or defined to -1, and can be checked at runtime with sysconf ().

This may be the case if the downstream distribution has switched from glibc crypt to libxcrypt.  When recompiling applications in such distributions the
 user must detect if _XOPEN_CRPYT is not available and include crypt.h for the function prototypes; otherwise libxcrypt is a ABI compatible drop-in replacement.
~~~

Note that this applies not only to crypt () but to encrypt () and setkey () also.

> diff --git a/man3/crypt.3 b/man3/crypt.3
> index 58c62d93d..8969ba0e7 100644
> --- a/man3/crypt.3
> +++ b/man3/crypt.3
> @@ -38,13 +38,9 @@
>  crypt, crypt_r \- password and data encryption
>  .SH SYNOPSIS
>  .nf
> -.BR "#define _XOPEN_SOURCE" "       /* See feature_test_macros(7) */"
> -.B #include <unistd.h>
> +.B #include <crypt.h>
>  .PP
>  .BI "char *crypt(const char *" key ", const char *" salt );
> -
> -.BR "#define _GNU_SOURCE" "         /* See feature_test_macros(7) */"
> -.B #include <crypt.h>
>  .PP
>  .BI "char *crypt_r(const char *" key ", const char *" salt ,
>  .BI "              struct crypt_data *" data );

This is only true for Fedora. We can carry it as a local patch if you wish.

(In reply to Nikos Mavrogiannopoulos from comment #16)
> There is also a typo with _XOPEN_CRPYT

Thanks. Notified upstream to fix it. Looks like Michael is about to commit it to man pages master.

Comment 18 Carlos O'Donell 2018-04-13 15:15:52 UTC
(In reply to Carlos O'Donell from comment #17)
> > diff --git a/man3/crypt.3 b/man3/crypt.3
> > index 58c62d93d..8969ba0e7 100644
> > --- a/man3/crypt.3
> > +++ b/man3/crypt.3
> > @@ -38,13 +38,9 @@
> >  crypt, crypt_r \- password and data encryption
> >  .SH SYNOPSIS
> >  .nf
> > -.BR "#define _XOPEN_SOURCE" "       /* See feature_test_macros(7) */"
> > -.B #include <unistd.h>
> > +.B #include <crypt.h>
> >  .PP
> >  .BI "char *crypt(const char *" key ", const char *" salt );
> > -
> > -.BR "#define _GNU_SOURCE" "         /* See feature_test_macros(7) */"
> > -.B #include <crypt.h>
> >  .PP
> >  .BI "char *crypt_r(const char *" key ", const char *" salt ,
> >  .BI "              struct crypt_data *" data );
> 
> This is only true for Fedora. We can carry it as a local patch if you wish.

To be clear the goal is to make it *not* Fedora-specific in the future, and remove crypt from glibc entirely so the API can evolve and be updated in an independent cadence.

Comment 19 František Zatloukal 2018-04-16 16:54:47 UTC
Discussed during the 2018-04-16 blocker review meeting: [1]

The decision to classify this bug as an RejectedBlocker was made:

"this is an interesting issue and may well need to be prominently communicated, but nothing cited so far comes close to breaking any of the release criteria. no functional impact on any part of Fedora itself has yet been noted at all."

[1] https://meetbot-raw.fedoraproject.org/fedora-blocker-review/2018-04-16/f28-blocker-review.2018-04-16-16.00.log.txt

Comment 20 František Zatloukal 2018-04-17 10:14:04 UTC
Discussed during the 2018-04-16 blocker review meeting: [1]

The decision to classify this bug as an RejectedFreezeException was made:

"this is rejected at least for now as there's no concrete proposal to actually change a package here, so no way to judge whether such a change would be appropriate. It can be re-proposed with a more specific suggestion if we get to one"

[1] https://meetbot-raw.fedoraproject.org/fedora-blocker-review/2018-04-16/f28-blocker-review.2018-04-16-16.00.log.txt


Note You need to log in before you can comment on or make changes to this bug.