| Message ID | 20171017120313.32229-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/string.c > +++ b/xen/arch/x86/string.c > @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n) > { > long d0, d1, d2; > > + if ( !n ) > + return; memmove() hopefully isn't on any really hot path, so the extra conditional shouldn't hurt much. Personally I think in cases like this, where the compiler would need to step out of its way in order to cause actually unexpected behavior, it is rather pointless to try to please a checking tool. Anyway, Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
>>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/string.c > +++ b/xen/arch/x86/string.c > @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n) > { > long d0, d1, d2; > > + if ( !n ) > + return; Actually - I can't see how this would build successfully: The function returns void *, not void. I'm taking the liberty to fix this (and also add unlikely()) while committing. Jan
On Fri, Oct 20, 2017 at 01:17:40AM -0600, Jan Beulich wrote: > >>> On 17.10.17 at 14:03, <roger.pau@citrix.com> wrote: > > --- a/xen/arch/x86/string.c > > +++ b/xen/arch/x86/string.c > > @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n) > > { > > long d0, d1, d2; > > > > + if ( !n ) > > + return; > > Actually - I can't see how this would build successfully: The function > returns void *, not void. I'm taking the liberty to fix this (and also > add unlikely()) while committing. Thanks and sorry. I tested this with clang 5.0 + ubsan enabled, but I have no idea why/how that compiled. Roger.
diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c index cd85a38e6d..19dcfe88ed 100644 --- a/xen/arch/x86/string.c +++ b/xen/arch/x86/string.c @@ -39,6 +39,9 @@ void *(memmove)(void *dest, const void *src, size_t n) { long d0, d1, d2; + if ( !n ) + return; + if ( dest < src ) return memcpy(dest, src, n);
ubsan in clang 5.0 complains with: (XEN) UBSAN: Undefined behaviour in string.c:50:28 (XEN) pointer overflow: (XEN) addition of unsigned offset to ffff830000100000 overflowed to ffff8300000fffff [...] (XEN) Xen call trace: (XEN) [<ffff82d0802dce0d>] ubsan.c#ubsan_epilogue+0xd/0xc0 (XEN) [<ffff82d0802de145>] __ubsan_handle_pointer_overflow+0xa5/0xe0 (XEN) [<ffff82d0803bf627>] memmove+0x67/0x80 (XEN) [<ffff82d080679f87>] page_alloc.c#bootmem_region_add+0x157/0x220 (XEN) [<ffff82d080679c66>] init_boot_pages+0x56/0x220 (XEN) [<ffff82d0806bcb9d>] __start_xen+0x2c2d/0x4b50 (XEN) [<ffff82d0802000f3>] __high_start+0x53/0x60 This is due to memmove doing a n-1+addr when n is 0. This is harmless because the loop is bounded to 0. Fix this by returning earlier when n is 0. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Julien Grall <julien.grall@arm.com> --- This is a harmless fix that shouldn't introduce any functional change, and hence it should be committed to 4.10 IMHO. --- xen/arch/x86/string.c | 3 +++ 1 file changed, 3 insertions(+)