[oe,meta-oe] xterm: Fix latent issue found with musl

Message ID 20191212202252.2786262-3-raj.khem@gmail.com
State New
Headers show
Series
  • [oe,meta-oe] xterm: Fix latent issue found with musl
Related show

Commit Message

Khem Raj Dec. 12, 2019, 8:22 p.m.
[YOCTO #13691]

Signed-off-by: Khem Raj <raj.khem@gmail.com>

Cc: Ross Burton <ross.burton@intel.com>
Cc: Armin Kuster <akuster808@gmail.com>
---
 .../xorg-app/xterm/posix_ptys.patch           | 29 +++++++++++++++++++
 .../recipes-graphics/xorg-app/xterm_351.bb    |  4 ++-
 2 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 meta-oe/recipes-graphics/xorg-app/xterm/posix_ptys.patch

-- 
2.24.1

-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel

Comments

Adrian Bunk Dec. 13, 2019, 4:37 a.m. | #1
On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:
>...

> +Upstream-Status: Pending


Not upstreamed OE-only musl patches create a technical debt.

>...

> +there is no test to define HAVE_GRANTPT_PTY_ISATTY


Is the definition in xterm_io.h not working?

>...

> +_POSIX_SOURCE is app-defined not system


This is true for musl, not for glibc.

The handling of _POSIX_SOURCE and _POSIX_VERSION differs between musl 
and glibc due to glibc supporting older POSIX versions, and glibc also 
supporting developers of portable code to select some specific older 
POSIX version.

>...

> +-#if defined(_POSIX_SOURCE) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)

> ++#if defined(_POSIX_VERSION) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)

> + 	    int pgrp = setsid();	/* variable may not be used... */

> + #else

> +         int pgrp = getpid();

>...


So this was caused by musl not supporting older versions of POSIX,
where _POSIX_SOURCE was part of the standard.

The proper fix would be an autoconf test for setsid().

cu
Adrian
-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel
Khem Raj Dec. 13, 2019, 9:55 p.m. | #2
On Thu, Dec 12, 2019 at 8:37 PM Adrian Bunk <bunk@stusta.de> wrote:
>

> On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:

> >...

> > +Upstream-Status: Pending

>

> Not upstreamed OE-only musl patches create a technical debt.

>

> >...

> > +there is no test to define HAVE_GRANTPT_PTY_ISATTY

>

> Is the definition in xterm_io.h not working?

>

> >...

> > +_POSIX_SOURCE is app-defined not system

>

> This is true for musl, not for glibc.

>

> The handling of _POSIX_SOURCE and _POSIX_VERSION differs between musl

> and glibc due to glibc supporting older POSIX versions, and glibc also

> supporting developers of portable code to select some specific older

> POSIX version.

>

No thats not right.
_POSIX_SOURCE (like all FTMs) is
defined by the application to request a feature/standards profile
it's not libc telling you "this is posix" or anything like that.
_POSIX_VERSION tells
you that

> >...

> > +-#if defined(_POSIX_SOURCE) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)

> > ++#if defined(_POSIX_VERSION) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)

> > +         int pgrp = setsid();        /* variable may not be used... */

> > + #else

> > +         int pgrp = getpid();

> >...

>

> So this was caused by musl not supporting older versions of POSIX,

> where _POSIX_SOURCE was part of the standard.

>


see above.

> The proper fix would be an autoconf test for setsid().

>


Thats a good idea. I have sent a v2 which adds a configure test got setsid()

> cu

> Adrian

-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel
Adrian Bunk Dec. 13, 2019, 10:58 p.m. | #3
On Fri, Dec 13, 2019 at 01:55:33PM -0800, Khem Raj wrote:
> On Thu, Dec 12, 2019 at 8:37 PM Adrian Bunk <bunk@stusta.de> wrote:

> > On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:

>...

> > > +_POSIX_SOURCE is app-defined not system

> >

> > This is true for musl, not for glibc.

> >

> > The handling of _POSIX_SOURCE and _POSIX_VERSION differs between musl

> > and glibc due to glibc supporting older POSIX versions, and glibc also

> > supporting developers of portable code to select some specific older

> > POSIX version.

> >

> No thats not right.

> _POSIX_SOURCE (like all FTMs) is

> defined by the application to request a feature/standards profile

> it's not libc telling you "this is posix" or anything like that.


It is also defined when _POSIX_C_SOURCE is defined, or _GNU_SOURCE,
or when the default non-strict gcc mode is used.

The latter means that it is in practice nearly always defined when the 
libc supports applications written against older versions of POSIX.

> _POSIX_VERSION tells you that

>...


_POSIX_VERSION being defined tells you that you have unistd.h included.

With glibc the value of _POSIX_VERSION depends on what feature/standards 
profile the application has requested.

cu
Adrian
-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel
Khem Raj Dec. 14, 2019, 5:55 a.m. | #4
On 12/13/19 2:58 PM, Adrian Bunk wrote:
> On Fri, Dec 13, 2019 at 01:55:33PM -0800, Khem Raj wrote:

>> On Thu, Dec 12, 2019 at 8:37 PM Adrian Bunk <bunk@stusta.de> wrote:

>>> On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:

>> ...

>>>> +_POSIX_SOURCE is app-defined not system

>>>

>>> This is true for musl, not for glibc.

>>>

>>> The handling of _POSIX_SOURCE and _POSIX_VERSION differs between musl

>>> and glibc due to glibc supporting older POSIX versions, and glibc also

>>> supporting developers of portable code to select some specific older

>>> POSIX version.

>>>

>> No thats not right.

>> _POSIX_SOURCE (like all FTMs) is

>> defined by the application to request a feature/standards profile

>> it's not libc telling you "this is posix" or anything like that.

> 

> It is also defined when _POSIX_C_SOURCE is defined, or _GNU_SOURCE,

> or when the default non-strict gcc mode is used.

> 

> The latter means that it is in practice nearly always defined when the

> libc supports applications written against older versions of POSIX.

> 

but these macros are not used in this case so its irrelevant here.

>> _POSIX_VERSION tells you that

>> ...

> 

> _POSIX_VERSION being defined tells you that you have unistd.h included.

> 

> With glibc the value of _POSIX_VERSION depends on what feature/standards

> profile the application has requested.


I would not look into how glibc does something and call it standard. It 
guards these definitions by its internal defines ( prefixed with __) 
which is internal to a libc implementation, applications are not exposed 
to private namespace.


> 

> cu

> Adrian

> 


-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel
Adrian Bunk Dec. 14, 2019, 2:20 p.m. | #5
On Fri, Dec 13, 2019 at 09:55:49PM -0800, Khem Raj wrote:
> On 12/13/19 2:58 PM, Adrian Bunk wrote:

> > On Fri, Dec 13, 2019 at 01:55:33PM -0800, Khem Raj wrote:

> > > On Thu, Dec 12, 2019 at 8:37 PM Adrian Bunk <bunk@stusta.de> wrote:

> > > > On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:

> > > ...

> > > > > +_POSIX_SOURCE is app-defined not system

> > > > 

> > > > This is true for musl, not for glibc.

> > > > 

> > > > The handling of _POSIX_SOURCE and _POSIX_VERSION differs between musl

> > > > and glibc due to glibc supporting older POSIX versions, and glibc also

> > > > supporting developers of portable code to select some specific older

> > > > POSIX version.

> > > > 

> > > No thats not right.

> > > _POSIX_SOURCE (like all FTMs) is

> > > defined by the application to request a feature/standards profile

> > > it's not libc telling you "this is posix" or anything like that.

> > 

> > It is also defined when _POSIX_C_SOURCE is defined, or _GNU_SOURCE,

> > or when the default non-strict gcc mode is used.

> > 

> > The latter means that it is in practice nearly always defined when the

> > libc supports applications written against older versions of POSIX.

> > 

> but these macros are not used in this case so its irrelevant here.


No macro has to be used, with gcc default settings _POSIX_SOURCE
is defined:

$ cat test.c
#include <unistd.h>
int tmp = _POSIX_SOURCE;
$ gcc -O2 -Wall -c test.c 
$

musl is different, since it does not aim at fully supporting 
applications that were written against an older version of
the POSIX standard.

> > > _POSIX_VERSION tells you that

> > > ...

> > 

> > _POSIX_VERSION being defined tells you that you have unistd.h included.

> > 

> > With glibc the value of _POSIX_VERSION depends on what feature/standards

> > profile the application has requested.

> 

> I would not look into how glibc does something and call it standard.


I would not look into how musl does something and call it standard.

I checked what musl and glibc do and what the standard says.

> It guards these definitions by its internal defines ( prefixed with __) which

> is internal to a libc implementation,  applications are not exposed to 

> private namespace.


_POSIX_SOURCE is defined in POSIX.1-1990 and mentioned in later versions 
of POSIX.

_POSIX_VERSION is defined in all(?) versions of POSIX.

cu
Adrian
-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel
Khem Raj Dec. 14, 2019, 3:24 p.m. | #6
On Sat, Dec 14, 2019 at 6:21 AM Adrian Bunk <bunk@stusta.de> wrote:

> On Fri, Dec 13, 2019 at 09:55:49PM -0800, Khem Raj wrote:

> > On 12/13/19 2:58 PM, Adrian Bunk wrote:

> > > On Fri, Dec 13, 2019 at 01:55:33PM -0800, Khem Raj wrote:

> > > > On Thu, Dec 12, 2019 at 8:37 PM Adrian Bunk <bunk@stusta.de> wrote:

> > > > > On Thu, Dec 12, 2019 at 12:22:52PM -0800, Khem Raj wrote:

> > > > ...

> > > > > > +_POSIX_SOURCE is app-defined not system

> > > > >

> > > > > This is true for musl, not for glibc.

> > > > >

> > > > > The handling of _POSIX_SOURCE and _POSIX_VERSION differs between

> musl

> > > > > and glibc due to glibc supporting older POSIX versions, and glibc

> also

> > > > > supporting developers of portable code to select some specific

> older

> > > > > POSIX version.

> > > > >

> > > > No thats not right.

> > > > _POSIX_SOURCE (like all FTMs) is

> > > > defined by the application to request a feature/standards profile

> > > > it's not libc telling you "this is posix" or anything like that.

> > >

> > > It is also defined when _POSIX_C_SOURCE is defined, or _GNU_SOURCE,

> > > or when the default non-strict gcc mode is used.

> > >

> > > The latter means that it is in practice nearly always defined when the

> > > libc supports applications written against older versions of POSIX.

> > >

> > but these macros are not used in this case so its irrelevant here.

>

> No macro has to be used, with gcc default settings _POSIX_SOURCE

> is defined:



Again this was what I said originally was not correct

>

>

> $ cat test.c

> #include <unistd.h>

> int tmp = _POSIX_SOURCE;

> $ gcc -O2 -Wall -c test.c

> $

>




> musl is different, since it does not aim at fully supporting

> applications that were written against an older version of

> the POSIX standard.

>

> > > > _POSIX_VERSION tells you that

> > > > ...

> > >

> > > _POSIX_VERSION being defined tells you that you have unistd.h included.

> > >

> > > With glibc the value of _POSIX_VERSION depends on what

> feature/standards

> > > profile the application has requested.

> >

> > I would not look into how glibc does something and call it standard.

>

> I would not look into how musl does something and call it standard.



Who said that effort is to find common solution

>

>

> I checked what musl and glibc do and what the standard says.

>

> > It guards these definitions by its internal defines ( prefixed with __)

> which

> > is internal to a libc implementation,  applications are not exposed to

> > private namespace.

>

> _POSIX_SOURCE is defined in POSIX.1-1990 and mentioned in later versions

> of POSIX.

>

> _POSIX_VERSION is defined in all(?) versions of POSIX.

>


Ok thank you

>

> cu

> Adrian

>

-- 
_______________________________________________
Openembedded-devel mailing list
Openembedded-devel@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-devel

Patch

diff --git a/meta-oe/recipes-graphics/xorg-app/xterm/posix_ptys.patch b/meta-oe/recipes-graphics/xorg-app/xterm/posix_ptys.patch
new file mode 100644
index 0000000000..54020d53e9
--- /dev/null
+++ b/meta-oe/recipes-graphics/xorg-app/xterm/posix_ptys.patch
@@ -0,0 +1,29 @@ 
+there is no test to define HAVE_GRANTPT_PTY_ISATTY and
+_POSIX_SOURCE is app-defined not system
+This fix ptys and launching xterm
+
+Upstream-Status: Pending
+
+Suggested By Rich Felker
+Signed-off-by: Khem Raj <raj.khem@gmail.com>
+
+--- a/main.c
++++ b/main.c
+@@ -2892,7 +2892,7 @@ get_pty(int *pty, char *from GCC_UNUSED)
+ 	close(opened_tty);
+ 	opened_tty = -1;
+     }
+-#elif defined(HAVE_POSIX_OPENPT) && defined(HAVE_PTSNAME) && defined(HAVE_GRANTPT_PTY_ISATTY)
++#elif defined(HAVE_POSIX_OPENPT) && defined(HAVE_PTSNAME)
+     if ((*pty = posix_openpt(O_RDWR)) >= 0) {
+ 	char *name = ptsname(*pty);
+ 	if (name != 0) {
+@@ -4040,7 +4040,7 @@ spawnXTerm(XtermWidget xw, unsigned line
+ 	    /*
+ 	     * now in child process
+ 	     */
+-#if defined(_POSIX_SOURCE) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)
++#if defined(_POSIX_VERSION) || defined(SVR4) || defined(__convex__) || defined(__SCO__) || defined(__QNX__)
+ 	    int pgrp = setsid();	/* variable may not be used... */
+ #else
+ 	    int pgrp = getpid();
diff --git a/meta-oe/recipes-graphics/xorg-app/xterm_351.bb b/meta-oe/recipes-graphics/xorg-app/xterm_351.bb
index 394d2cb9de..abfda8a5fa 100644
--- a/meta-oe/recipes-graphics/xorg-app/xterm_351.bb
+++ b/meta-oe/recipes-graphics/xorg-app/xterm_351.bb
@@ -4,7 +4,9 @@  DEPENDS = "libxaw xorgproto libxext libxau libxinerama libxpm ncurses"
 
 LIC_FILES_CHKSUM = "file://xterm.h;beginline=3;endline=31;md5=c7faceb872d90115e7c0ad90e90c390d"
 
-SRC_URI = "http://invisible-mirror.net/archives/${BPN}/${BP}.tgz"
+SRC_URI = "http://invisible-mirror.net/archives/${BPN}/${BP}.tgz \
+           file://posix_ptys.patch \
+          "
 
 SRC_URI[md5sum] = "a07edfbee2e2f4c6a9ddbf834fa4bbec"
 SRC_URI[sha256sum] = "760a8a10221c9c9744afd86db87c7ad95bbf9be4f5f525fecf39125f0d2a6e16"