[ruby-core:69662] [Ruby trunk - Bug #11270] [Open] Coverity Scan warns out-of-bounds access in ext/socket

Issue #11270 has been updated by Yusuke Endoh.

Status changed from Feedback to Open

Akira Tanaka wrote:
> arg.alen is initialized as sizeof(union_sockaddr) and
> modified by recvfrom() which is less than or equal to sizeof(union_sockaddr).
> 
> rai is rb_addrinfo_t and rai->addr is union_sockaddr.
> 
> So the memcpy() doesn't overflow.

I think that Coverity Scan is warning against `sa`, not `rai`.  `sa` is `&arg.buf.addr`, not `&arg.buf`.  If it were `&arg.buf`, there is certainly no problem.

Honestly I'm not sure the C language specification: is it guaranteed that a pointer to a field of a union and a pointer to the union itself?  In short, `(void*)&arg.buf.addr == (void*)&arg.buf`?  If it is guaranteed, there is no problem.  But I couldn't find the guarantee from the specification.


> > I don't think this inconsistency will cause actual harm, but it would be better to fix.
> 
> Do you have an idea to fix it?
> 
> I guess the inconsistency is caused by "struct sockaddr" is used as a type for generic socket addresses
> but actually a fixed length buffer which may be not enough for some addresses.
> It is a Unix tradition and too dificult to fix.

I'm not familiar with socket apis.  Do you mean that the apis are ill-designed so that we cannot use them in the strict C language?  If so, I agree that it is difficult to fix. 

-- 
Yusuke Endoh <[e-mail]>

----------------------------------------
Bug #11270: Coverity Scan warns out-of-bounds access in ext/socket
https://bugs.ruby-lang.org/issues/11270#change-53022

* Author: Yusuke Endoh
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
Hello,

Coverity Scan warns ext/socket/init.c and raddrinfo.c.

`rsock_s_recvfrom` in ext/socket/init.c does:

    arg.alen = (socklen_t)sizeof(arg.buf);

then calls `rsock_io_socket_addrinfo`:

    return rb_assoc_new(str, rsock_io_socket_addrinfo(sock, &arg.buf.addr, arg.alen));

`rsock_io_socket_addrinfo` indirectly calls `init_addrinfo` in ext/socket/raddrinfo.c.
(`rsock_io_socket_addrinfo` -> `rsock_fd_socket_addrinfo` -> `rsock_addrinfo_new` -> `init_addrinfo`)

`init_addrinfo` does:

    memcpy((void *)&rai->addr, (void *)sa, len);

Note that `sa` is `&arg.buf.addr`, and `len` is `arg.alen`.  `&arg.buf.addr` is a pointer to sockaddr, and `arg.len` is `sizeof(union_sockaddr)`, not `sizeof(sockaddr)`, which is indeed inconsistent.

I don't think this inconsistency will cause actual harm, but it would be better to fix.

-- 
Yusuke Endoh <[e-mail]>



-- 
https://bugs.ruby-lang.org/