[vlc-devel] [PATCH 1/2] compat: Workaround sendmsg bug on some ArmV8 Android devices

[Hugo Beauzée-Luyssen]

On Fri, Mar 29, 2019, at 2:08 PM, Rémi Denis-Courmont wrote:
> Overriding errno on success looks like a bad idea. It wouldn't be the 
> first time that it breaks error handling in nonobvious manner.
> 

It felt like an additional safety but I'm fine not touching errno and just assuming that ~0 on 32 or 64bits is an error.
That being said errno can be set in case of success, so probing errno after a call to sendmsg for a non-sendmsg error seems like a bad idea in any case.

> And FWIW, I don't agree with the assessment that bugs in libc syscall 
> wrappers cannot be coded around - especially if you target only one 
> specific ISA.
> 

I can't see anything obviously wrong in the syscall wrapper: https://android.googlesource.com/platform/bionic.git/+/refs/heads/master/libc/arch-arm64/syscalls/sendmsg.S
But I can't say I'm fluent with Assembly *at all*
I can't see anything in the libc's logs that points me to a fix there, hence the feeling that the issue is more on the kernel level.

> Le 29 mars 2019 15:02:28 GMT+02:00, "Hugo Beauzée-Luyssen" 
> <hugo at beauzee.fr> a écrit : On some devices (at least up to Android 6) 
> sendmsg will return its error
> > code on 32bits instead of 64.
> > Most likely some code is storing the ssize_t on a size_t, getting rid of
> > the sign bit and therefore of the sign extension afterward.
> > This causes us to use 0xFFFFFFFF as a very large number of bytes instead
> > of an error.
> > This patch works around the issue and assumes that 0xFFFFFFFF with a non
> > 0 errno is an error and forces it to be returned on 64bits. include/vlc_network.h | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/include/vlc_network.h b/include/vlc_network.h
> > index 7cc384587a..477d351cc8 100644
> > --- a/include/vlc_network.h
> > +++ b/include/vlc_network.h
> > @@ -280,6 +280,29 @@ static inline int net_GetPeerAddress( int fd, char *address, int *port )
> >  
> >  VLC_API char *vlc_getProxyUrl(const char *);
> >  
> > +#if defined(__ANDROID__) && defined(__aarch64__)
> > +
> > +/**
> > + * Since we bumped the NDK version from 14 to 18, some devices (at least up to
> > + * Android 6) are returning errors on 4 bytes, even though ssize_t is actually
> > + * 8 bytes. This causes `value < 0` checks to yield false, and consider the value
> > + * as a non error.
> > + * As the patch lies in either the NDK or the Android kernel, or the device libc
> > + * we can only work around it. If errno is not 0 & we receive -1, on 32bits or
> > + * 64bits, we assume an error was returned.
> > + */
> > +static inline ssize_t vlc_sendmsg(int fd, const struct msghdr *msg, int flags)
> > +{
> > +    errno = 0;
> > +    ssize_t ret = sendmsg(fd, msg, flags);
> > +    if ((ret < 0 || ret == 0xFFFFFFFF) && errno != 0)
> > +        return -1;
> > +    return ret;
> > +}
> > +#define sendmsg(a, b, c) vlc_sendmsg(a,b,c)
> > +
> > +#endif
> > +
> >  # ifdef __cplusplus
> >  }
> >  # endif
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser 
> ma brièveté. 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr