> On 11 Sep 2021, at 21:16, Martijn van Duren <openbsd+t...@list.imperialat.at>
> wrote:
>
> On Sat, 2021-09-11 at 04:05 +0300, Vitaliy Makkoveev wrote:
>> On Fri, Sep 10, 2021 at 07:40:58PM +0200, Alexander Bluhm wrote:
>>> Hi,
>>>
>>> After converting syslogd messages to iov, UDP can use sendmsg(2)
>>> instead of snprintf().
>>>
>>> ok?
>>>
>>
>> I have one question below, but the diff is ok by mvs@ if it is not
>> significant.
>>
>> It is expected we could drop iov[6].iov_len bytes more from `msg' but
>> keep newline stuff when message size exceeds MAX_UDPMSG? The original
>> code has the different behaviour.
>
> Maybe I'm misreading, but the iov[6] is set in the final switch
> statement, which lacks the F_FORWUDP case, so it drops to default,
> which has an empty string, not a newline.
I missed this, thanks.
>
> As I already mailed bluhm@ in private; I'm more curious about the
> else case for if (iov[5].iov_len > l). In this case iov[0..4]
> still overflow MAX_UDPMSG and we also drop the entire "msg"
> itself, which leaves basically no useful information.
> Since we only prepend pribuf (13 bytes), lasttime (33 bytes + 1 space),
> and prevhost/localhostname (HOST_NAME_MAX=255 + 1 bytes + 1 space) we
> can at most reach 304 bytes, which is a lot shorter than the current
> MAX_UDPMSG of 1180 bytes.
> If we trust that noone will be stupid enough to blindly change
> MAX_UDPMSG to <304 bytes, or start bloating the preemble >876 bytes I
> don't see a reason to add this case.
We could have the calculations above as a commentary near MAX_UDPMSG
definition to prevent this.
> If it's a genuine concern then I think it would make more sense
> to just drop the message altogether, since everything that could be
> useful is already lost.
>>
>>> bluhm
>>>
>>> Index: usr.sbin/syslogd/syslogd.c
>>> ===================================================================
>>> RCS file: /cvs/src/usr.sbin/syslogd/syslogd.c,v
>>> retrieving revision 1.269
>>> diff -u -p -r1.269 syslogd.c
>>> --- usr.sbin/syslogd/syslogd.c 10 Sep 2021 15:18:36 -0000 1.269
>>> +++ usr.sbin/syslogd/syslogd.c 10 Sep 2021 17:36:45 -0000
>>> @@ -1897,6 +1897,7 @@ void
>>> fprintlog(struct filed *f, int flags, char *msg)
>>> {
>>> struct iovec iov[IOVCNT], *v;
>>> + struct msghdr msghdr;
>>> int l, retryonce;
>>> char line[LOG_MAXLINE + 1], pribuf[13], greetings[500], repbuf[80];
>>> char ebuf[ERRBUFSIZE];
>>> @@ -2037,20 +2038,22 @@ fprintlog(struct filed *f, int flags, ch
>>>
>>> case F_FORWUDP:
>>> log_debug(" %s", f->f_un.f_forw.f_loghost);
>>> - l = snprintf(line, MINIMUM(MAX_UDPMSG + 1, sizeof(line)),
>>> - "%s%s%s%s%s%s%s", (char *)iov[0].iov_base,
>>> - (char *)iov[1].iov_base, (char *)iov[2].iov_base,
>>> - (char *)iov[3].iov_base, (char *)iov[4].iov_base,
>>> - (char *)iov[5].iov_base, (char *)iov[6].iov_base);
>>> - if (l < 0)
>>> - l = strlcpy(line, iov[5].iov_base, sizeof(line));
>>> - if (l >= sizeof(line))
>>> - l = sizeof(line) - 1;
>>> - if (l >= MAX_UDPMSG + 1)
>>> - l = MAX_UDPMSG;
>>> - if (sendto(f->f_file, line, l, 0,
>>> - (struct sockaddr *)&f->f_un.f_forw.f_addr,
>>> - f->f_un.f_forw.f_addr.ss_len) != l) {
>>> + l = iov[0].iov_len + iov[1].iov_len + iov[2].iov_len +
>>> + iov[3].iov_len + iov[4].iov_len + iov[5].iov_len +
>>> + iov[6].iov_len;
>>> + if (l > MAX_UDPMSG) {
>>> + l -= MAX_UDPMSG;
>>> + if (iov[5].iov_len > l)
>>> + iov[5].iov_len -= l;
>>> + else
>>> + iov[5].iov_len = 0;
>>> + }
>>> + memset(&msghdr, 0, sizeof(msghdr));
>>> + msghdr.msg_name = &f->f_un.f_forw.f_addr;
>>> + msghdr.msg_namelen = f->f_un.f_forw.f_addr.ss_len;
>>> + msghdr.msg_iov = iov;
>>> + msghdr.msg_iovlen = IOVCNT;
>>> + if (sendmsg(f->f_file, &msghdr, 0) == -1) {
>>> switch (errno) {
>>> case EADDRNOTAVAIL:
>>> case EHOSTDOWN:
>>> @@ -2063,7 +2066,7 @@ fprintlog(struct filed *f, int flags, ch
>>> break;
>>> default:
>>> f->f_type = F_UNUSED;
>>> - log_warn("sendto \"%s\"",
>>> + log_warn("sendmsg to \"%s\"",
>>> f->f_un.f_forw.f_loghost);
>>> break;
>>> }
