Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Tue, Apr 14, 2015 at 04:39:44PM +0100, Dimitris Papastamos wrote: On Tue, Apr 14, 2015 at 04:33:56PM +0100, Connor Lane Smith wrote: On 14 April 2015 at 13:50, Dimitris Papastamos s...@2f30.org wrote: dst and src are required to be valid objects even if n is 0, otherwise this is undefined behaviour. I looked this up in C11. Seems to be the case: 7.24.2.1.2. The memcpy function copies n characters from the object pointed to by s2 into the object pointed to by s1. 6.2.6.1.2. Except for bit-fields, objects are composed of contiguous sequences of one or more bytes, ... 124) ... there are no pointers to or arrays of bit-field objects. An object pointed to by s1 or s2 is guaranteed to be at least 1 byte long, so memcpy is free to dereference the first byte of either, whether or not n 0. I think it's a mistake that a libc actually go ahead and do this, but the C standard suggests that it is permitted, so we should certainly do the check. Yup, well it happens only with the optimized version of memmove() in OpenBSD. Not sure if this was intended or not. In any case, it is valid for libc to assume that src/dst is at least 1 byte long even if n is 0 as you say. Yeah, see here, under null objects: http://www.tedunangst.com/flak/post/zero-size-objects This should be reverted. -- Omar
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
This should be reverted. Yeah, I agree. I also think we should put some comment saying that if i == 0 then it is possible to have some of the undefined cases where src == NULL || dst == NULL || src == dst. Some volunter to write the patch?
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
See attached patch.
From f6c18450a99875647d85f633fef00e72a0c1577b Mon Sep 17 00:00:00 2001
From: sin s...@2f30.org
Date: Wed, 15 Apr 2015 09:43:06 +0100
Subject: [PATCH] Fix memmove() invocation with src/dst being NULL
This fixes a segmentation fault on some systems.
---
st.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/st.c b/st.c
index b1d3791..bb8c365 100644
--- a/st.c
+++ b/st.c
@@ -2788,8 +2788,11 @@ tresize(int col, int row) {
free(term.line[i]);
free(term.alt[i]);
}
- memmove(term.line, term.line + i, row * sizeof(Line));
- memmove(term.alt, term.alt + i, row * sizeof(Line));
+ /* ensure that both src and dst are not NULL */
+ if (i 0) {
+ memmove(term.line, term.line + i, row * sizeof(Line));
+ memmove(term.alt, term.alt + i, row * sizeof(Line));
+ }
for(i += row; i term.row; i++) {
free(term.line[i]);
free(term.alt[i]);
--
1.8.4
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Tue, Apr 14, 2015 at 12:55 PM, Gregor Best g...@ring0.de wrote: The cause seems to be that for bot `term.line` and `term.alt` are NULL at this point. While this does mean that even with a `len` parameter of 0, the `dst` pointer gets touched, I don't think it's ever right to call either `memcpy` or `memmove` with a NULL destination pointer. I just tested glibc's memmove. The results are: dst == NULL, n 0: segfault src == NULL, n 0: segfault if dst or src are NULL and n is 0 there is no segfault. Fascinating.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On 14 April 2015 at 13:50, Dimitris Papastamos s...@2f30.org wrote: dst and src are required to be valid objects even if n is 0, otherwise this is undefined behaviour. I looked this up in C11. Seems to be the case: 7.24.2.1.2. The memcpy function copies n characters from the object pointed to by s2 into the object pointed to by s1. 6.2.6.1.2. Except for bit-fields, objects are composed of contiguous sequences of one or more bytes, ... 124) ... there are no pointers to or arrays of bit-field objects. An object pointed to by s1 or s2 is guaranteed to be at least 1 byte long, so memcpy is free to dereference the first byte of either, whether or not n 0. I think it's a mistake that a libc actually go ahead and do this, but the C standard suggests that it is permitted, so we should certainly do the check. cls
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Tue, Apr 14, 2015 at 04:33:56PM +0100, Connor Lane Smith wrote: On 14 April 2015 at 13:50, Dimitris Papastamos s...@2f30.org wrote: dst and src are required to be valid objects even if n is 0, otherwise this is undefined behaviour. I looked this up in C11. Seems to be the case: 7.24.2.1.2. The memcpy function copies n characters from the object pointed to by s2 into the object pointed to by s1. 6.2.6.1.2. Except for bit-fields, objects are composed of contiguous sequences of one or more bytes, ... 124) ... there are no pointers to or arrays of bit-field objects. An object pointed to by s1 or s2 is guaranteed to be at least 1 byte long, so memcpy is free to dereference the first byte of either, whether or not n 0. I think it's a mistake that a libc actually go ahead and do this, but the C standard suggests that it is permitted, so we should certainly do the check. Yup, well it happens only with the optimized version of memmove() in OpenBSD. Not sure if this was intended or not. In any case, it is valid for libc to assume that src/dst is at least 1 byte long even if n is 0 as you say.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Tue, Apr 14, 2015 at 01:43:56PM +0200, Silvan Jegen wrote: On Tue, Apr 14, 2015 at 12:55 PM, Gregor Best g...@ring0.de wrote: The cause seems to be that for bot `term.line` and `term.alt` are NULL at this point. While this does mean that even with a `len` parameter of 0, the `dst` pointer gets touched, I don't think it's ever right to call either `memcpy` or `memmove` with a NULL destination pointer. I just tested glibc's memmove. The results are: dst == NULL, n 0: segfault src == NULL, n 0: segfault if dst or src are NULL and n is 0 there is no segfault. Fascinating. dst and src are required to be valid objects even if n is 0, otherwise this is undefined behaviour. The fact that it doesn't crash on this implementation doesn't mean anything.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
The assembly is rather irrelevant in this case. Let's write the code the way it is most understandable and clear. Yeah, I totally agree with you here. I'd remove the if, memmove is equivalent to a non-op when the offset is 0. But the problem now is to decide what is more understable. I prefer an explicit if, because it alerts to the programmer about this case, but the no-if version is shorter and without indentation, that usually means that it is more understable. The fact you can pass a size=0 to memcpy and memmove is one of the most stupid things you can find in the C standard, because it is illogical and remove a lot of optimizations in some processors, so I don't know if honour this rule is something we should do. Regards,
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
Applied, thanks.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On April 12, 2015 10:56:40 AM CEST, non...@inventati.org wrote: On Sun, Apr 12, 2015 at 10:41:36AM +0200, Silvan Jegen wrote: I was thinking about this option too but in that case you would be calling memmove with an identical src and dst when i == 0. The man page for glibc memmove(3) does not mention anything about this being a noop so I am not sure that is a good thing to do. The memory areas may overlap: copying takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest. src == dest is just a case of overlapping areas, it must work. It is not a nop though, but two copy operations. Even with i=0 the function call might be more costly than a simple je.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
Heyhey On Sun, Apr 12, 2015 at 08:56:40AM +, non...@inventati.org wrote: On Sun, Apr 12, 2015 at 10:41:36AM +0200, Silvan Jegen wrote: I was thinking about this option too but in that case you would be calling memmove with an identical src and dst when i == 0. The man page for glibc memmove(3) does not mention anything about this being a noop so I am not sure that is a good thing to do. The memory areas may overlap: copying takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest. src == dest is just a case of overlapping areas, it must work. I don't doubt that it will work :) I just wonder if it really will be copying the data around into a temporary array for no reason when i == 0 (either in glibc or in another libc). I don't think it is worth removing this if and potentially copying data around to no avail. At the same time I don't think this function is called very often so in practice it will not matter in either case. I just wanted to point it out so people can take it into consideration before deciding to apply your patches.
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Sun, Apr 12, 2015 at 08:29:55AM +, noname wrote:
---
st.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/st.c b/st.c
index 109122e..f183803 100644
--- a/st.c
+++ b/st.c
@@ -2788,10 +2788,8 @@ tresize(int col, int row) {
free(term.line[i]);
free(term.alt[i]);
}
- if(i 0) {
- memmove(term.line, term.line + i, row * sizeof(Line));
- memmove(term.alt, term.alt + i, row * sizeof(Line));
- }
+ memmove(term.line, term.line + i, row * sizeof(Line));
+ memmove(term.alt, term.alt + i, row * sizeof(Line));
I was thinking about this option too but in that case you would be
calling memmove with an identical src and dst when i == 0. The man page
for glibc memmove(3) does not mention anything about this being a noop
so I am not sure that is a good thing to do.
for(i += row; i term.row; i++) {
free(term.line[i]);
free(term.alt[i]);
--
1.8.4
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Sun, Apr 12, 2015 at 10:41:36AM +0200, Silvan Jegen wrote: I was thinking about this option too but in that case you would be calling memmove with an identical src and dst when i == 0. The man page for glibc memmove(3) does not mention anything about this being a noop so I am not sure that is a good thing to do. The memory areas may overlap: copying takes place as though the bytes in src are first copied into a temporary array that does not overlap src or dest, and the bytes are then copied from the temporary array to dest. src == dest is just a case of overlapping areas, it must work.
[dev] [st] [PATCH 4/3] tresize: remove unnecessary if
---
st.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/st.c b/st.c
index 109122e..f183803 100644
--- a/st.c
+++ b/st.c
@@ -2788,10 +2788,8 @@ tresize(int col, int row) {
free(term.line[i]);
free(term.alt[i]);
}
- if(i 0) {
- memmove(term.line, term.line + i, row * sizeof(Line));
- memmove(term.alt, term.alt + i, row * sizeof(Line));
- }
+ memmove(term.line, term.line + i, row * sizeof(Line));
+ memmove(term.alt, term.alt + i, row * sizeof(Line));
for(i += row; i term.row; i++) {
free(term.line[i]);
free(term.alt[i]);
--
1.8.4
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Sun, 12 Apr 2015 11:36:00 +0200 koneu kone...@googlemail.com wrote: It is not a nop though, but two copy operations. Even with i=0 the function call might be more costly than a simple je. The assembly is rather irrelevant in this case. Let's write the code the way it is most understandable and clear. I'd remove the if, memmove is equivalent to a non-op when the offset is 0. Cheers FRIGN -- FRIGN d...@frign.de
Re: [dev] [st] [PATCH 4/3] tresize: remove unnecessary if
On Sun, Apr 12, 2015 at 11:35:42AM +0200, Silvan Jegen wrote: I just wonder if it really will be copying the data around into a temporary array for no reason when i == 0 (either in glibc or in another libc). Well, it does not really move data to temporary array, memmove just chooses the right direction so it does not destroy data. glibc indeed wastes CPU cycles if dst == src, However, musl does not: http://git.musl-libc.org/cgit/musl/tree/src/string/memmove.c
