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
