MUSL fun

MUSL fun

[Brendan Heading]

Hello all,

When compiling against the musl C library, an issue comes up in
lib/commands/toolcontext.c. There's some logic that reopens and
updates stdin and stdout in the event that their file descriptors are
in O_WRONLY mode.

================
if (!_reopen_stream(stdin, STDIN_FILENO, "r", "stdin", &new_stream))
    goto_out;
stdin = new_stream;
================

musl's view is that stdin/stdout/stderr are read only and are never
meant to be replaced by programs, so they declare them as const, which
means the above section (and similar sections) don't compile.

In theory a straightforward fix would be to do something like:

if (!freopen(NULL, "r", stdin))
     goto out;

.. but I suspect there is a good reason why this isn't done. Can
anyone shed any light on this ? If not I will submit a patch.

regards

Brendan

MUSL fun

[Bernd Kuhls]

Am Tue, 15 Sep 2015 17:43:58 +0100 schrieb Brendan Heading:

> In theory a straightforward fix would be to do something like:
> 
> if (!freopen(NULL, "r", stdin))
>      goto out;
> 
> .. but I suspect there is a good reason why this isn't done. Can anyone
> shed any light on this ? If not I will submit a patch.

Hi Brendan,

the buildroot project also supports musl, the build of lvm2 fails due to 
the problem you described. Did you find a working solution for the 
problem? A patch I submitted to the buildroot project, based on patches I 
found at the Alpinelinux project, raised some questions I am unable to 
answer: http://patchwork.ozlabs.org/patch/573519/

Regards, Bernd

[Buildroot] [lvm-devel] MUSL fun

[Brendan Heading]

Bernd,

[cc buildroot]

Like you I'd been investigating this issue from the buildroot end.
Since October I've had no spare time to look at buildroot fixes.

I was hoping for an explanation about why LVM implemented the above
behaviour, but I never got one, and I didn't fancy working on a patch
that would never get accepted when other projects were more open ..

I think the best approach is probably just to add the freopen() patch
as Romain suggested and keep the patch available in buildroot in the
hope that we can build support for LVM to accept it.

Brendan


On 13 February 2016 at 18:48, Bernd Kuhls <bernd.kuhls@t-online.de> wrote:
> Am Tue, 15 Sep 2015 17:43:58 +0100 schrieb Brendan Heading:
>
>> In theory a straightforward fix would be to do something like:
>>
>> if (!freopen(NULL, "r", stdin))
>>      goto out;
>>
>> .. but I suspect there is a good reason why this isn't done. Can anyone
>> shed any light on this ? If not I will submit a patch.
>
> Hi Brendan,
>
> the buildroot project also supports musl, the build of lvm2 fails due to
> the problem you described. Did you find a working solution for the
> problem? A patch I submitted to the buildroot project, based on patches I
> found at the Alpinelinux project, raised some questions I am unable to
> answer: http://patchwork.ozlabs.org/patch/573519/
>
> Regards, Bernd
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

MUSL fun

[Brendan Heading]

Bernd,

[cc buildroot]

Like you I'd been investigating this issue from the buildroot end.
Since October I've had no spare time to look at buildroot fixes.

I was hoping for an explanation about why LVM implemented the above
behaviour, but I never got one, and I didn't fancy working on a patch
that would never get accepted when other projects were more open ..

I think the best approach is probably just to add the freopen() patch
as Romain suggested and keep the patch available in buildroot in the
hope that we can build support for LVM to accept it.

Brendan


On 13 February 2016 at 18:48, Bernd Kuhls <bernd.kuhls@t-online.de> wrote:
> Am Tue, 15 Sep 2015 17:43:58 +0100 schrieb Brendan Heading:
>
>> In theory a straightforward fix would be to do something like:
>>
>> if (!freopen(NULL, "r", stdin))
>>      goto out;
>>
>> .. but I suspect there is a good reason why this isn't done. Can anyone
>> shed any light on this ? If not I will submit a patch.
>
> Hi Brendan,
>
> the buildroot project also supports musl, the build of lvm2 fails due to
> the problem you described. Did you find a working solution for the
> problem? A patch I submitted to the buildroot project, based on patches I
> found at the Alpinelinux project, raised some questions I am unable to
> answer: http://patchwork.ozlabs.org/patch/573519/
>
> Regards, Bernd
>
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel

Re: [linux-lvm] [lvm-devel] MUSL fun

[Zdenek Kabelac]

Dne 13.2.2016 v 20:22 Brendan Heading napsal(a):
> Bernd,
>
> [cc buildroot]
>
> Like you I'd been investigating this issue from the buildroot end.
> Since October I've had no spare time to look at buildroot fixes.
>
> I was hoping for an explanation about why LVM implemented the above
> behaviour, but I never got one, and I didn't fancy working on a patch
> that would never get accepted when other projects were more open ..
>
> I think the best approach is probably just to add the freopen() patch
> as Romain suggested and keep the patch available in buildroot in the
> hope that we can build support for LVM to accept it.
>
> Brendan
>
>
> On 13 February 2016 at 18:48, Bernd Kuhls <bernd.kuhls-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org> wrote:
>> Am Tue, 15 Sep 2015 17:43:58 +0100 schrieb Brendan Heading:
>>
>>> In theory a straightforward fix would be to do something like:
>>>
>>> if (!freopen(NULL, "r", stdin))
>>>       goto out;
>>>
>>> .. but I suspect there is a good reason why this isn't done. Can anyone
>>> shed any light on this ? If not I will submit a patch.
>>
>> Hi Brendan,
>>
>> the buildroot project also supports musl, the build of lvm2 fails due to
>> the problem you described. Did you find a working solution for the
>> problem? A patch I submitted to the buildroot project, based on patches I
>> found at the Alpinelinux project, raised some questions I am unable to
>> answer: http://patchwork.ozlabs.org/patch/573519/
>>
>> Regards, Bernd


Let's start from the beginning -

-- 
commands/toolcontext.c: In function 'create_toolcontext':
commands/toolcontext.c:1796:10: error: assignment of read-only variable 'stdin'
     stdin = new_stream;
-- 

So now - from where comes the idea  of  'stdin' being read-only variable ?
Looking at some POSIX spec e.g.:
http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html

there is no sign of having this read-only var.

So what kind of OS this actually is ?


There is a reason from lvm2 implementation here - freopen() is not giving 
application full control over internal buffer allocation and we need to be 
sure we lock-in memory for critical section - and some glibc versions are 
allocating buffers here via 'mmap' call.

That said - there could be accepted a patch checking in configure  for
'read-only' stdin  - and using then #ifdef compilation that would
replace use of  internal lvm2 reopen code with libc function.

But the currently posted patch may possibly cause some other regressions.

Regards

Zdenek

MUSL fun

[Zdenek Kabelac]

Dne 13.2.2016 v 20:22 Brendan Heading napsal(a):
> Bernd,
>
> [cc buildroot]
>
> Like you I'd been investigating this issue from the buildroot end.
> Since October I've had no spare time to look at buildroot fixes.
>
> I was hoping for an explanation about why LVM implemented the above
> behaviour, but I never got one, and I didn't fancy working on a patch
> that would never get accepted when other projects were more open ..
>
> I think the best approach is probably just to add the freopen() patch
> as Romain suggested and keep the patch available in buildroot in the
> hope that we can build support for LVM to accept it.
>
> Brendan
>
>
> On 13 February 2016 at 18:48, Bernd Kuhls <bernd.kuhls-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org> wrote:
>> Am Tue, 15 Sep 2015 17:43:58 +0100 schrieb Brendan Heading:
>>
>>> In theory a straightforward fix would be to do something like:
>>>
>>> if (!freopen(NULL, "r", stdin))
>>>       goto out;
>>>
>>> .. but I suspect there is a good reason why this isn't done. Can anyone
>>> shed any light on this ? If not I will submit a patch.
>>
>> Hi Brendan,
>>
>> the buildroot project also supports musl, the build of lvm2 fails due to
>> the problem you described. Did you find a working solution for the
>> problem? A patch I submitted to the buildroot project, based on patches I
>> found at the Alpinelinux project, raised some questions I am unable to
>> answer: http://patchwork.ozlabs.org/patch/573519/
>>
>> Regards, Bernd


Let's start from the beginning -

-- 
commands/toolcontext.c: In function 'create_toolcontext':
commands/toolcontext.c:1796:10: error: assignment of read-only variable 'stdin'
     stdin = new_stream;
-- 

So now - from where comes the idea  of  'stdin' being read-only variable ?
Looking at some POSIX spec e.g.:
http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html

there is no sign of having this read-only var.

So what kind of OS this actually is ?


There is a reason from lvm2 implementation here - freopen() is not giving 
application full control over internal buffer allocation and we need to be 
sure we lock-in memory for critical section - and some glibc versions are 
allocating buffers here via 'mmap' call.

That said - there could be accepted a patch checking in configure  for
'read-only' stdin  - and using then #ifdef compilation that would
replace use of  internal lvm2 reopen code with libc function.

But the currently posted patch may possibly cause some other regressions.

Regards

Zdenek

[Buildroot] [lvm-devel] MUSL fun

[Brendan Heading]

> commands/toolcontext.c: In function 'create_toolcontext':
> commands/toolcontext.c:1796:10: error: assignment of read-only variable
> 'stdin'
>     stdin = new_stream;
> --
>
> So now - from where comes the idea  of  'stdin' being read-only variable ?
> Looking at some POSIX spec e.g.:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html
>
> there is no sign of having this read-only var.
>
> So what kind of OS this actually is ?

musl's rather conservative interpretation appears to be that since the
values of stdin, stdout and stderr are explicitly defined by POSIX, it
is implicit that the variables holding these values should be const.

http://www.openwall.com/lists/musl/2013/03/18/1

There's not a lot of point in debating the rights or wrongs of this.
If it were me, I'd take a pragmatic approach, and if it were me I'd
make this a configurable option, but this defeats musl's stated
objectives and they won't accept a patch to change it.

> There is a reason from lvm2 implementation here - freopen() is not giving
> application full control over internal buffer allocation and we need to be
> sure we lock-in memory for critical section - and some glibc versions are
> allocating buffers here via 'mmap' call.
>
> That said - there could be accepted a patch checking in configure  for
> 'read-only' stdin  - and using then #ifdef compilation that would
> replace use of  internal lvm2 reopen code with libc function.

I had a suspicion you would have a good reason.

On first glance your proposal sounds like a good compromise and one
that would be worth exploring.

Brendan

Re: [linux-lvm] [lvm-devel] MUSL fun

[Brendan Heading]

> commands/toolcontext.c: In function 'create_toolcontext':
> commands/toolcontext.c:1796:10: error: assignment of read-only variable
> 'stdin'
>     stdin = new_stream;
> --
>
> So now - from where comes the idea  of  'stdin' being read-only variable ?
> Looking at some POSIX spec e.g.:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html
>
> there is no sign of having this read-only var.
>
> So what kind of OS this actually is ?

musl's rather conservative interpretation appears to be that since the
values of stdin, stdout and stderr are explicitly defined by POSIX, it
is implicit that the variables holding these values should be const.

http://www.openwall.com/lists/musl/2013/03/18/1

There's not a lot of point in debating the rights or wrongs of this.
If it were me, I'd take a pragmatic approach, and if it were me I'd
make this a configurable option, but this defeats musl's stated
objectives and they won't accept a patch to change it.

> There is a reason from lvm2 implementation here - freopen() is not giving
> application full control over internal buffer allocation and we need to be
> sure we lock-in memory for critical section - and some glibc versions are
> allocating buffers here via 'mmap' call.
>
> That said - there could be accepted a patch checking in configure  for
> 'read-only' stdin  - and using then #ifdef compilation that would
> replace use of  internal lvm2 reopen code with libc function.

I had a suspicion you would have a good reason.

On first glance your proposal sounds like a good compromise and one
that would be worth exploring.

Brendan

MUSL fun

[Brendan Heading]

> commands/toolcontext.c: In function 'create_toolcontext':
> commands/toolcontext.c:1796:10: error: assignment of read-only variable
> 'stdin'
>     stdin = new_stream;
> --
>
> So now - from where comes the idea  of  'stdin' being read-only variable ?
> Looking at some POSIX spec e.g.:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/stdin.html
>
> there is no sign of having this read-only var.
>
> So what kind of OS this actually is ?

musl's rather conservative interpretation appears to be that since the
values of stdin, stdout and stderr are explicitly defined by POSIX, it
is implicit that the variables holding these values should be const.

http://www.openwall.com/lists/musl/2013/03/18/1

There's not a lot of point in debating the rights or wrongs of this.
If it were me, I'd take a pragmatic approach, and if it were me I'd
make this a configurable option, but this defeats musl's stated
objectives and they won't accept a patch to change it.

> There is a reason from lvm2 implementation here - freopen() is not giving
> application full control over internal buffer allocation and we need to be
> sure we lock-in memory for critical section - and some glibc versions are
> allocating buffers here via 'mmap' call.
>
> That said - there could be accepted a patch checking in configure  for
> 'read-only' stdin  - and using then #ifdef compilation that would
> replace use of  internal lvm2 reopen code with libc function.

I had a suspicion you would have a good reason.

On first glance your proposal sounds like a good compromise and one
that would be worth exploring.

Brendan

[Buildroot] [lvm-devel] MUSL fun

[Brendan Heading]

>> There is a reason from lvm2 implementation here - freopen() is not giving
>> application full control over internal buffer allocation and we need to be
>> sure we lock-in memory for critical section - and some glibc versions are
>> allocating buffers here via 'mmap' call.
>>
>> That said - there could be accepted a patch checking in configure  for
>> 'read-only' stdin  - and using then #ifdef compilation that would
>> replace use of  internal lvm2 reopen code with libc function.
>
> I had a suspicion you would have a good reason.
>
> On first glance your proposal sounds like a good compromise and one
> that would be worth exploring.

As an alternative idea, what about changing the codebase not to use
stdin/stdout/stderr, using it's own file descriptor names and calling
dup2() as appropriate to set them up on startup ?

Just flying the kite - I haven't looked to see how painful this might be.

Brendan

Re: [linux-lvm] [lvm-devel] MUSL fun

[Brendan Heading]

>> There is a reason from lvm2 implementation here - freopen() is not giving
>> application full control over internal buffer allocation and we need to be
>> sure we lock-in memory for critical section - and some glibc versions are
>> allocating buffers here via 'mmap' call.
>>
>> That said - there could be accepted a patch checking in configure  for
>> 'read-only' stdin  - and using then #ifdef compilation that would
>> replace use of  internal lvm2 reopen code with libc function.
>
> I had a suspicion you would have a good reason.
>
> On first glance your proposal sounds like a good compromise and one
> that would be worth exploring.

As an alternative idea, what about changing the codebase not to use
stdin/stdout/stderr, using it's own file descriptor names and calling
dup2() as appropriate to set them up on startup ?

Just flying the kite - I haven't looked to see how painful this might be.

Brendan

MUSL fun

[Brendan Heading]

>> There is a reason from lvm2 implementation here - freopen() is not giving
>> application full control over internal buffer allocation and we need to be
>> sure we lock-in memory for critical section - and some glibc versions are
>> allocating buffers here via 'mmap' call.
>>
>> That said - there could be accepted a patch checking in configure  for
>> 'read-only' stdin  - and using then #ifdef compilation that would
>> replace use of  internal lvm2 reopen code with libc function.
>
> I had a suspicion you would have a good reason.
>
> On first glance your proposal sounds like a good compromise and one
> that would be worth exploring.

As an alternative idea, what about changing the codebase not to use
stdin/stdout/stderr, using it's own file descriptor names and calling
dup2() as appropriate to set them up on startup ?

Just flying the kite - I haven't looked to see how painful this might be.

Brendan

Re: [linux-lvm] [lvm-devel] MUSL fun

[Zdenek Kabelac]

Dne 13.2.2016 v 22:31 Brendan Heading napsal(a):
>>> There is a reason from lvm2 implementation here - freopen() is not giving
>>> application full control over internal buffer allocation and we need to be
>>> sure we lock-in memory for critical section - and some glibc versions are
>>> allocating buffers here via 'mmap' call.
>>>
>>> That said - there could be accepted a patch checking in configure  for
>>> 'read-only' stdin  - and using then #ifdef compilation that would
>>> replace use of  internal lvm2 reopen code with libc function.
>>
>> I had a suspicion you would have a good reason.
>>
>> On first glance your proposal sounds like a good compromise and one
>> that would be worth exploring.
>
> As an alternative idea, what about changing the codebase not to use
> stdin/stdout/stderr, using it's own file descriptor names and calling
> dup2() as appropriate to set them up on startup ?
>
> Just flying the kite - I haven't looked to see how painful this might be.

Hi

Well don't know anyone who would play with this approach which is several 
orders in magnitude more difficult then the plain and simple #ifdef.

I also assume we may probably even overwrite value of stdin via tricky pointer 
magic so compiler will not be able to spot mods of stdin in buildtime.

Regards

Zdenek

MUSL fun

[Zdenek Kabelac]

Dne 13.2.2016 v 22:31 Brendan Heading napsal(a):
>>> There is a reason from lvm2 implementation here - freopen() is not giving
>>> application full control over internal buffer allocation and we need to be
>>> sure we lock-in memory for critical section - and some glibc versions are
>>> allocating buffers here via 'mmap' call.
>>>
>>> That said - there could be accepted a patch checking in configure  for
>>> 'read-only' stdin  - and using then #ifdef compilation that would
>>> replace use of  internal lvm2 reopen code with libc function.
>>
>> I had a suspicion you would have a good reason.
>>
>> On first glance your proposal sounds like a good compromise and one
>> that would be worth exploring.
>
> As an alternative idea, what about changing the codebase not to use
> stdin/stdout/stderr, using it's own file descriptor names and calling
> dup2() as appropriate to set them up on startup ?
>
> Just flying the kite - I haven't looked to see how painful this might be.

Hi

Well don't know anyone who would play with this approach which is several 
orders in magnitude more difficult then the plain and simple #ifdef.

I also assume we may probably even overwrite value of stdin via tricky pointer 
magic so compiler will not be able to spot mods of stdin in buildtime.

Regards

Zdenek