svn commit: r286144 - head/usr.bin/wall

[Pedro Giffuni]

On 01/08/2015 10:01 a.m., Pedro Giffuni wrote:
>
>
> On 08/01/15 09:34, Eric van Gyzen wrote:
>> On 7/31/15 8:29 PM, Pedro F. Giffuni wrote:
>>> Author: pfg
>>> Date: Sat Aug  1 01:29:55 2015
>>> New Revision: 286144
>>> URL: https://svnweb.freebsd.org/changeset/base/286144
>>>
>>> Log:
>>>    Buffer overflow in wall(1).
>>>
>>>    Revert r286102 and apply a cleaner fix.
>>>    Tested for overflows by FORTIFY_SOURCE GSoC (with clang).
>>>
>>>    Suggested by:    bde
>>>    Reviewed by:    Oliver Pinter
>>>    Tested by:    Oliver Pinter
>>>    MFC after:    3 days
>>>
>>> Modified:
>>>    head/usr.bin/wall/ttymsg.c
>>>
>>> Modified: head/usr.bin/wall/ttymsg.c
>>> ============================================================================== 
>>>
>>>
>>> --- head/usr.bin/wall/ttymsg.c    Fri Jul 31 23:40:18 2015 (r286143)
>>> +++ head/usr.bin/wall/ttymsg.c    Sat Aug  1 01:29:55 2015 (r286144)
>>> @@ -62,7 +62,7 @@ ttymsg(struct iovec *iov, int iovcnt, co
>>>       struct iovec localiov[7];
>>>       ssize_t left, wret;
>>>       int cnt, fd;
>>> -    char device[MAXNAMLEN] = _PATH_DEV;
>>> +    char device[MAXNAMLEN];
>>>       static char errbuf[1024];
>>>       char *p;
>>>       int forked;
>>> @@ -71,8 +71,9 @@ ttymsg(struct iovec *iov, int iovcnt, co
>>>       if (iovcnt > (int)(sizeof(localiov) / sizeof(localiov[0])))
>>>           return ("too many iov's (change code in wall/ttymsg.c)");
>>>
>>> -    strlcat(device, line, sizeof(device));
>>> +    strlcpy(device, _PATH_DEV, sizeof(device));
>>>       p = device + sizeof(_PATH_DEV) - 1;
>>> +    strlcpy(p, line, sizeof(device) - sizeof(_PATH_DEV));
>>
>> You're probably already sick of this change, but I would encourage this
>> instead:
>>
>>      strlcat(device, line, sizeof(device));
>>
>
> It looks exactly like the code I just replaced ;).
>


>> Your current code works, and it's even more efficient than strlcat.
>


Actually it looks like there may be some consensus towards reverting this
as the previous version was more readable. Since the code still works better
than when there was an overflow, let's leave it for a couple of days, maybe
interested developers will come with a better change.

Pedro.