[Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Richard Henderson]

The pointer may be unaligned, so we must use our routines for that.
At the same time, we might as well use the big-endian version
instead of ntohs.

This fixes sparc64 host SIGBUS during pxe boot.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 slirp/slirp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 322edf51eb..a116f43878 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
     if (pkt_len < ETH_HLEN)
         return;
 
-    proto = ntohs(*(uint16_t *)(pkt + 12));
+    proto = lduw_be_p(pkt + 12);
     switch(proto) {
     case ETH_P_ARP:
         arp_input(slirp, pkt, pkt_len);
-- 
2.17.2

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Peter Maydell]

On Wed, 26 Dec 2018 at 03:44, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The pointer may be unaligned, so we must use our routines for that.
> At the same time, we might as well use the big-endian version
> instead of ntohs.
>
> This fixes sparc64 host SIGBUS during pxe boot.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  slirp/slirp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Philippe Mathieu-Daudé]

Le mer. 26 déc. 2018 04:43, Richard Henderson <richard.henderson@linaro.org>
a écrit :

> The pointer may be unaligned, so we must use our routines for that.
> At the same time, we might as well use the big-endian version
> instead of ntohs.
>
> This fixes sparc64 host SIGBUS during pxe boot.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

---
>  slirp/slirp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 322edf51eb..a116f43878 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int
> pkt_len)
>      if (pkt_len < ETH_HLEN)
>          return;
>
> -    proto = ntohs(*(uint16_t *)(pkt + 12));
> +    proto = lduw_be_p(pkt + 12);
>      switch(proto) {
>      case ETH_P_ARP:
>          arp_input(slirp, pkt, pkt_len);
> --
> 2.17.2
>
>
>

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Samuel Thibault]

Hello,

Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit:
> The pointer may be unaligned, so we must use our routines for that.
> At the same time, we might as well use the big-endian version
> instead of ntohs.
> 
> This fixes sparc64 host SIGBUS during pxe boot.

I'm not at ease with applying this, when Marc-André is trying to make
slirp an external library...  I'd rather apply the change below, could
somebody review it?

Samuel


slirp: Avoid unaligned 16bit memory access

pkt parameter may be unaligned, so we must access it byte-wise.

This fixes sparc64 host SIGBUS during pxe boot.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

diff --git a/slirp/slirp.c b/slirp/slirp.c
index ab2fc4eb8b..0e41d5aedf 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
     if (pkt_len < ETH_HLEN)
         return;
 
-    proto = ntohs(*(uint16_t *)(pkt + 12));
+    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
     switch(proto) {
     case ETH_P_ARP:
         arp_input(slirp, pkt, pkt_len);

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Richard Henderson]

On 1/17/19 10:50 AM, Samuel Thibault wrote:
> Hello,
> 
> Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit:
>> The pointer may be unaligned, so we must use our routines for that.
>> At the same time, we might as well use the big-endian version
>> instead of ntohs.
>>
>> This fixes sparc64 host SIGBUS during pxe boot.
> 
> I'm not at ease with applying this, when Marc-André is trying to make
> slirp an external library...  I'd rather apply the change below, could
> somebody review it?

Fair.

> -    proto = ntohs(*(uint16_t *)(pkt + 12));
> +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];

This works for me too, though I note unnecessary parenthesis around the cast.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Philippe Mathieu-Daudé]

On 1/17/19 12:50 AM, Samuel Thibault wrote:
> Hello,
> 
> Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit:
>> The pointer may be unaligned, so we must use our routines for that.
>> At the same time, we might as well use the big-endian version
>> instead of ntohs.
>>
>> This fixes sparc64 host SIGBUS during pxe boot.
> 
> I'm not at ease with applying this, when Marc-André is trying to make
> slirp an external library...  I'd rather apply the change below, could
> somebody review it?
> 
> Samuel
> 
> 
> slirp: Avoid unaligned 16bit memory access
> 
> pkt parameter may be unaligned, so we must access it byte-wise.
> 
> This fixes sparc64 host SIGBUS during pxe boot.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index ab2fc4eb8b..0e41d5aedf 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>      if (pkt_len < ETH_HLEN)
>          return;
>  
> -    proto = ntohs(*(uint16_t *)(pkt + 12));
> +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
>      switch(proto) {
>      case ETH_P_ARP:
>          arp_input(slirp, pkt, pkt_len);

What about using memcpy?

-- >8 --
@@ -846,12 +846,13 @@ static void arp_input(Slirp *slirp, const uint8_t
*pkt, int pkt_len)
 void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
 {
     struct mbuf *m;
-    int proto;
+    uint16_t proto;

     if (pkt_len < ETH_HLEN)
         return;

-    proto = ntohs(*(uint16_t *)(pkt + 12));
+    memcpy(&proto, pkt + 12, sizeof(proto)); /* Avoid unaligned 16bit
access */
+    proto = ntohs(proto);
     switch(proto) {
     case ETH_P_ARP:
         arp_input(slirp, pkt, pkt_len);
---

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Samuel Thibault]

Philippe Mathieu-Daudé, le jeu. 17 janv. 2019 14:16:16 +0100, a ecrit:
> On 1/17/19 12:50 AM, Samuel Thibault wrote:
> > Hello,
> > 
> > Richard Henderson, le mer. 26 déc. 2018 14:42:54 +1100, a ecrit:
> >> The pointer may be unaligned, so we must use our routines for that.
> >> At the same time, we might as well use the big-endian version
> >> instead of ntohs.
> >>
> >> This fixes sparc64 host SIGBUS during pxe boot.
> > 
> > I'm not at ease with applying this, when Marc-André is trying to make
> > slirp an external library...  I'd rather apply the change below, could
> > somebody review it?
> > 
> > Samuel
> > 
> > 
> > slirp: Avoid unaligned 16bit memory access
> > 
> > pkt parameter may be unaligned, so we must access it byte-wise.
> > 
> > This fixes sparc64 host SIGBUS during pxe boot.
> > 
> > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> > 
> > diff --git a/slirp/slirp.c b/slirp/slirp.c
> > index ab2fc4eb8b..0e41d5aedf 100644
> > --- a/slirp/slirp.c
> > +++ b/slirp/slirp.c
> > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> >      if (pkt_len < ETH_HLEN)
> >          return;
> >  
> > -    proto = ntohs(*(uint16_t *)(pkt + 12));
> > +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
> >      switch(proto) {
> >      case ETH_P_ARP:
> >          arp_input(slirp, pkt, pkt_len);
> 
> What about using memcpy?

Well, it looks to me even more confusing than doing the shifts :)

> -- >8 --
> @@ -846,12 +846,13 @@ static void arp_input(Slirp *slirp, const uint8_t
> *pkt, int pkt_len)
>  void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>  {
>      struct mbuf *m;
> -    int proto;
> +    uint16_t proto;
> 
>      if (pkt_len < ETH_HLEN)
>          return;
> 
> -    proto = ntohs(*(uint16_t *)(pkt + 12));
> +    memcpy(&proto, pkt + 12, sizeof(proto)); /* Avoid unaligned 16bit
> access */
> +    proto = ntohs(proto);
>      switch(proto) {
>      case ETH_P_ARP:
>          arp_input(slirp, pkt, pkt_len);
> ---
> 

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Peter Maydell]

On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 1/17/19 12:50 AM, Samuel Thibault wrote:
> > --- a/slirp/slirp.c
> > +++ b/slirp/slirp.c
> > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> >      if (pkt_len < ETH_HLEN)
> >          return;
> >
> > -    proto = ntohs(*(uint16_t *)(pkt + 12));
> > +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
> >      switch(proto) {
> >      case ETH_P_ARP:
> >          arp_input(slirp, pkt, pkt_len);
>
> What about using memcpy?

We should use whatever the new libslirp wants to consistently
use as its mechanism for loading unaligned data. I don't
suppose this is the only place where it ever needs to do this.

Personally I would vote for having libslirp have versions of
the ld*_p functions, because they solve the problem in a
clear and correct way. But that's up to Marc-André really.

(As you can see from clang build logs:
https://travis-ci.org/qemu/qemu/jobs/480813811
slirp/ has a lot of as-yet-unfixed "takes address of packed
member" bugs, which suggest it's a bit slapdash with
alignment. Running it under the clang sanitizer to look
for runtime alignment errors might also be interesting.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Marc-André Lureau]

Hi

On Thu, Jan 17, 2019 at 5:56 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > On 1/17/19 12:50 AM, Samuel Thibault wrote:
> > > --- a/slirp/slirp.c
> > > +++ b/slirp/slirp.c
> > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> > >      if (pkt_len < ETH_HLEN)
> > >          return;
> > >
> > > -    proto = ntohs(*(uint16_t *)(pkt + 12));
> > > +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
> > >      switch(proto) {
> > >      case ETH_P_ARP:
> > >          arp_input(slirp, pkt, pkt_len);
> >
> > What about using memcpy?
>
> We should use whatever the new libslirp wants to consistently
> use as its mechanism for loading unaligned data. I don't
> suppose this is the only place where it ever needs to do this.
>
> Personally I would vote for having libslirp have versions of
> the ld*_p functions, because they solve the problem in a
> clear and correct way. But that's up to Marc-André really.

I think I would go with a copy of qemu bswap.h, unless there is an
equivalent in glib (I don't think so) or gnulib? Or other standard
compiler solution.

It's somehow surprising me that there is no goto solution :).

> (As you can see from clang build logs:
> https://travis-ci.org/qemu/qemu/jobs/480813811
> slirp/ has a lot of as-yet-unfixed "takes address of packed
> member" bugs, which suggest it's a bit slapdash with
> alignment. Running it under the clang sanitizer to look
> for runtime alignment errors might also be interesting.)
>
> thanks
> -- PMM



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Marc-André Lureau]

Hi

On Fri, Jan 18, 2019 at 3:25 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Jan 17, 2019 at 5:56 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 17 Jan 2019 at 13:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > On 1/17/19 12:50 AM, Samuel Thibault wrote:
> > > > --- a/slirp/slirp.c
> > > > +++ b/slirp/slirp.c
> > > > @@ -851,7 +851,7 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> > > >      if (pkt_len < ETH_HLEN)
> > > >          return;
> > > >
> > > > -    proto = ntohs(*(uint16_t *)(pkt + 12));
> > > > +    proto = (((uint16_t) pkt[12]) << 8) + pkt[13];
> > > >      switch(proto) {
> > > >      case ETH_P_ARP:
> > > >          arp_input(slirp, pkt, pkt_len);
> > >
> > > What about using memcpy?
> >
> > We should use whatever the new libslirp wants to consistently
> > use as its mechanism for loading unaligned data. I don't
> > suppose this is the only place where it ever needs to do this.
> >
> > Personally I would vote for having libslirp have versions of
> > the ld*_p functions, because they solve the problem in a
> > clear and correct way. But that's up to Marc-André really.
>
> I think I would go with a copy of qemu bswap.h, unless there is an
> equivalent in glib (I don't think so) or gnulib? Or other standard
> compiler solution.
>

The GStreamer solution is also quite readable.
https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/master/gst/gstutils.h#L165


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[Peter Maydell]

On Fri, 18 Jan 2019 at 11:37, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> The GStreamer solution is also quite readable.
> https://gitlab.freedesktop.org/gstreamer/gstreamer/blob/master/gst/gstutils.h#L165

It has the disadvantage that you can never legitimately
set GST_HAVE_UNALIGNED_ACCESS (because it is always
undefined-behaviour by the C standard) and the slow-path
versions are slow-and-clunky. Using malloc like the QEMU
approach has the advantage of telling the compiler
what you're doing so it can emit the optimal code on
systems where it works.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] slirp: Use lduw_be_p in slirp_input

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181226034254.17842-1-richard.henderson@linaro.org/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      crypto/block.o
  CC      crypto/block-qcow.o
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20181226034254.17842-1-richard.henderson@linaro.org/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com