Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t

From: Mattias Andrée <maandree_AT_kth.se>
Date: Fri, 2 Dec 2016 13:34:49 +0100

On Fri, 2 Dec 2016 13:22:16 +0100
Laslo Hunhold <dev_AT_frign.de> wrote:

> On Thu, 1 Dec 2016 22:50:20 -0800
> Michael Forney <mforney_AT_mforney.org> wrote:
>
> Hey Michael,
>
> > memmove moves a number of bytes, not pointers, so if
> > you passed a number of arguments that is larger than
> > the pointer byte size, you could end up crashing or
> > skipping the install of a file and installing another
> > twice.
>
> well-observed, nice find!
>
> > Also, argv was never decreased to match the moved
> > arguments, so the -t parameter was added in the NULL
> > argv slot.
>
> > - memmove(argv - 1, argv, argc);
> > + argv = memmove(argv - 1, argv, argc *
> > sizeof(*argv));
>
> I got to admit that this piece of code is really ugly to
> begin with. We _must not_ use memmove here as we invoke
> undefined behaviour, given the two memory regions overlap.
> Also, it's really bad style to call the value "tflag",
> given it's not an int but actually a char pointer to the
> name of the target folder, so "tflag" should rather be
> called "target". Same applies to the other values.
>
> I am wondering if we even need this. I mean, we already
> "consume" the target directory in ARGBEGIN ... ARGEND and
> thus are only left with sources in argv.
>
> Moreover, I generally question the existence of some
> flags for install (1), like -s to strip symbols. Do we
> really need this?

-s needs to exist because people actually use it in makefiles.
But it could be made into a dummy flag, it might even be
preferable, because then you don't have to remove use of -s
from makefile if you want the package you install to retain
symbols.

> Especially with non-standardized tools
> like install(1), we need to be careful not to swallow the
> waste that has accumulated over the years. The usage of
> this tool has become so complicated that using it
> properly becomes harder and harder with the number of
> options growing. What are your thoughts? Is the -s flag
> direly needed for some applications? Should we just
> silently ignore it?

If it's actually need, the package could call strip on
the binaries that fail, or the developer can call strip
explicitly. If it's even possible symbols could be problem,
it's so rare that it wouldn't be much of a headache to
deal with.

>
> Cheers
>
> Laslo
>


Received on Fri Dec 02 2016 - 13:34:49 CET

This archive was generated by hypermail 2.3.0 : Fri Dec 02 2016 - 13:36:25 CET