I've switched it to a LOG_UNIMP, similar to to the one several lines
below. I will follow up with a change to switch this to an assert as
recommended.
On Tue, Jan 28, 2020 at 9:07 AM Laurent Vivier <laur...@vivier.eu> wrote:
>
> Le 28/01/2020 à 17:53, Alex Bennée a écrit :
> >
> > Laurent Vivier <laur...@vivier.eu> writes:
> >
> >> Le 17/01/2020 à 20:28, Josh Kunz a écrit :
> >>> Since most calls to `gemu_log` are actually logging unimplemented
> >>> features,
> >>> this change replaces most non-strace calls to `gemu_log` with calls to
> >>> `qemu_log_mask(LOG_UNIMP, ...)`. This allows the user to easily log to
> >>> a file, and to mask out these log messages if they desire.
> >>>
> >>> Note: This change is slightly backwards incompatible, since now these
> >>> "unimplemented" log messages will not be logged by default.
> >>
> >> This is a good incompatibility as these messages were unexpected by the
> >> tools catching stderr. They don't happen on "real" systems.
> >>
> >> ...
> >>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> >>> index 249e4b95fc..629f3a21b5 100644
> >>> --- a/linux-user/syscall.c
> >>> +++ b/linux-user/syscall.c
> >>> @@ -1545,20 +1545,18 @@ static inline abi_long target_to_host_cmsg(struct
> >>> msghdr *msgh,
> >>> - sizeof(struct target_cmsghdr);
> >>>
> >>> space += CMSG_SPACE(len);
> >>> - if (space > msgh->msg_controllen) {
> >>> - space -= CMSG_SPACE(len);
> >>> - /* This is a QEMU bug, since we allocated the payload
> >>> - * area ourselves (unlike overflow in host-to-target
> >>> - * conversion, which is just the guest giving us a buffer
> >>> - * that's too small). It can't happen for the payload types
> >>> - * we currently support; if it becomes an issue in future
> >>> - * we would need to improve our allocation strategy to
> >>> - * something more intelligent than "twice the size of the
> >>> - * target buffer we're reading from".
> >>> - */
> >>> - gemu_log("Host cmsg overflow\n");
> >>> - break;
> >>> - }
> >>> +
> >>> + /*
> >>> + * This is a QEMU bug, since we allocated the payload
> >>> + * area ourselves (unlike overflow in host-to-target
> >>> + * conversion, which is just the guest giving us a buffer
> >>> + * that's too small). It can't happen for the payload types
> >>> + * we currently support; if it becomes an issue in future
> >>> + * we would need to improve our allocation strategy to
> >>> + * something more intelligent than "twice the size of the
> >>> + * target buffer we're reading from".
> >>> + */
> >>> + assert(space > msgh->msg_controllen && "Host cmsg overflow");
>
> Should it be in fact :
>
> assert(space <= msgh->msg_controllen && "Host cmsg overflow");
>
> >>> if (tswap32(target_cmsg->cmsg_level) == TARGET_SOL_SOCKET) {
> >>> cmsg->cmsg_level = SOL_SOCKET;
> >>
> >> Could you move this to a separate patch: you are not using qemu_log()
> >> here and I'm not convinced that crashing is better than ignoring the
> >> remaining part of the buffer.
> >
> > I suggested it should be an assert in the first place. It certainly
> > makes sense to keep it in a separate patch though. I guess you could
> > argue for:
> >
> > qemu_log_mask(LOG_UNIMP, "%s: unhandled message size");
> >
> > but is it really better to partially work and continue? It seems like
> > you would get more subtle hidden bugs.
>
> ok, you're right. crash seems to be a better solution.
>
> So, we only need to move this change to a separate patch.
>
> Thanks,
> Laurent
>
