Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Thu, 1 Dec 2016 22:50:20 -0800 Michael Forney 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. > > Also, argv was never decreased to match the moved arguments, so the -t > parameter was added in the NULL argv slot. after taking another look at this patch, it totally makes sense. Thanks for your submission! I applied it, but need to note here that this is a pretty hacky contraption. :P Cheers Laslo -- Laslo Hunhold
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, 2 Dec 2016 13:54:22 +0100 Anselm R Garbe wrote: > Hi there, > > On 2 December 2016 at 13:34, Mattias Andrée > wrote: > > 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. > > IMHO install has always been a bad idea and a good > example of violating the Unix philosophy. Hence my > suggestion would be to ban it completely from sbase. I would love to see install banned, but I think it's unpractical. Virtual all makefiles must be modified, and that is going to be a lot of work for any distro that chooses to uses sbase and what to have a lot of package available for their users. > > -Anselm > pgpJca9I0LNdq.pgp Description: OpenPGP digital signature
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, 2 Dec 2016 13:54:22 +0100 Anselm R Garbe wrote: Hey Anselm, > IMHO install has always been a bad idea and a good example of > violating the Unix philosophy. Hence my suggestion would be to ban it > completely from sbase. the reason why it was added in the first place is because it's used in many places, and it "makes sense" because people are too lazy to call mkdir -p before trying to install something somewhere, or strip, or chmod and so on. My suggestion would be to really think hard which flags we need, which flags we should just keep as "empty flags" for compatibility (like -s) and which ones we should remove. Usage across the operating systems is really different. For instance, the BSD installs don't even have the t-flag. Cheers Laslo -- Laslo Hunhold
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
Hi there, On 2 December 2016 at 13:34, Mattias Andrée wrote: > 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. IMHO install has always been a bad idea and a good example of violating the Unix philosophy. Hence my suggestion would be to ban it completely from sbase. -Anselm
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, 2 Dec 2016 13:22:16 +0100 Laslo Hunhold wrote: > On Thu, 1 Dec 2016 22:50:20 -0800 > Michael Forney 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 > pgpNRibYdD7YF.pgp Description: OpenPGP digital signature
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, 2 Dec 2016 12:24:54 + Dimitris Papastamos wrote: Hey Dimitris, > it is not undefined behaviour, you are thinking of memcpy thanks for pointing it out. I just realized this after sending the mail. Is there a good reason for the memmove in this place? Maybe I'm missing something. Cheers Laslo -- Laslo Hunhold
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, 2 Dec 2016 13:22:16 +0100 Laslo Hunhold wrote: > We _must not_ use memmove here as we invoke undefined behaviour, > given the two memory regions overlap. Nevermind this part, sorry. I mixed it up with memcpy. Still, do we need this here? -- Laslo Hunhold
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Fri, Dec 02, 2016 at 01:22:16PM +0100, Laslo Hunhold wrote: > On Thu, 1 Dec 2016 22:50:20 -0800 > Michael Forney 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. it is not undefined behaviour, you are thinking of memcpy
Re: [hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
On Thu, 1 Dec 2016 22:50:20 -0800 Michael Forney 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? 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? Cheers Laslo -- Laslo Hunhold
[hackers] [sbase] [PATCH] xinstall: Fix broken memmove with -t
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.
Also, argv was never decreased to match the moved arguments, so the -t
parameter was added in the NULL argv slot.
---
xinstall.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xinstall.c b/xinstall.c
index bf921fb..869237a 100644
--- a/xinstall.c
+++ b/xinstall.c
@@ -222,7 +222,7 @@ main(int argc, char *argv[])
}
if (tflag) {
- memmove(argv - 1, argv, argc);
+ argv = memmove(argv - 1, argv, argc * sizeof(*argv));
argv[argc++] = tflag;
}
if (tflag || argc > 2) {
--
2.10.2
