pp_accept: fix potential out-of-bounds read on oversized accept() addrlen

[crystarm]

Summary: This PR fixes a potential out-of-bounds read in pp_accept (pp_sys.c).

Problem: PerlSock_accept_cloexec() (via accept()/accept4()) can return an addrlen larger than the supplied buffer size when the peer address is truncated. The previous code used the returned len directly in PUSHp(namebuf, len).

Fix: Clamp len to sizeof(namebuf) before calling PUSHp.

Impact: No behavior change for valid lengths. Prevents reading past namebuf when addrlen is oversized.

Context: Change was motivated by static analysis finding. BUFFER_OVERFLOW.PROC pp_sys.c:[2631:10].log

This set of changes does not require a perldelta entry.

[jkeenan]

Please run perl Porting/updateAUTHORS.pl to provide your name and email address. Then run make test_harness and make sure the entire test suite passes before re-submitting.

[richardleach]

When this situation arises, is the value addrlen accurate for what was returned from the accept() call?

• If not, it sounds like it should be PerlSock_accept_cloexec that is fixed instead?
• If it is, and the problem is just that namebuf isn't big enough to store the peer address, then it sounds like there will still be a data truncation problem, which doesn't seem desirable? If this is what's happening, then maybe the fix should be to ensure when perl is built that MAXPATHLEN will always be large enough.

[richardleach]

When this situation arises, is the value addrlen accurate for what was returned from the accept() call?

• If not, it sounds like it should be PerlSock_accept_cloexec that is fixed instead?
• If it is, and the problem is just that namebuf isn't big enough to store the peer address, then it sounds like there will still be a data truncation problem, which doesn't seem desirable? If this is what's happening, then maybe the fix should be to ensure when perl is built that MAXPATHLEN will always be large enough.

Or change the declaration of namebuf to use something suitable other than MAXPATHLEN, since that seems to be designed to handle file path lengths.

I think namebuf has to be big enough for sizeof(struct sockaddr_storage)? If so, a quick squint at the nest of macros behind it suggests that MAXPATHLEN is likely to be plenty big enough - though there could be some corner cases hiding in there.

[Leont]

I am not so sure this is true. It might be for the syscall but that doesn't mean it's true for the library call.

But this could easily be experimentally proven.

[tonycoz]

When this situation arises, is the value addrlen accurate for what was returned from the accept() call?

from man accept on Linux:

       The addrlen argument is a value-result argument: the  caller  must  ini‐
       tialize it to contain the size (in bytes) of the structure pointed to by
       addr; on return it will contain the actual size of the peer address.
 
       The  returned  address is truncated if the buffer provided is too small;
       in this case, addrlen will return a value greater than was  supplied  to
       the call.

so yes, it looks like the resulting addrlen can be larger than the supplied buffer (which I wasn't aware of.)

https://news.ycombinator.com/item?id=22868968 claims some systems can have much longer paths for unix sockets than sockaddr_un fits.

If this is a problem, it also appears to be a problem for getpeername() and getsockname().

The main problem I can see is that it will silently truncate the address if it doesn't fit, which I don't see a simple fix for. We could loop on getpeername() if we see it truncated, but implementations vary enough I'm not sure I'd trust that.

I am not so sure this is true. It might be for the syscall but that doesn't mean it's true for the library call.

From looking at the glibc source accept() in glibc is just the system call.

HP-UX (yeah, it's old) has a similar note

POSIX:

address_len
Either a null pointer, if address is a null pointer, or a pointer to a socklen_t object which on input specifies the length of the supplied sockaddr structure, and on output specifies the length of the stored address.
If address is not a null pointer, the address of the peer for the accepted connection shall be stored in the sockaddr structure pointed to by address, and the length of this address shall be stored in the object pointed to by address_len.

If the actual length of the address is greater than the length of the supplied sockaddr structure, the stored address shall be truncated.
(which is a bit less clear, "on output specifies the length of the stored address.")

[crystarm]

Please run perl Porting/updateAUTHORS.pl to provide your name and email address. Then run make test_harness and make sure the entire test suite passes before re-submitting.

Thanks, I'll do it before updating the PR.

[crystarm]

@richardleach
Thank you for the questions! ദ്ദി(˵ •̀ ᴗ - ˵ ) ✧

I dug a bit into this and made a few small C reproducers. I'll attach them here as perl5-24434-experiments.zip

In short: yes, the returned addrlen can be “accurate” in the sense of describing the full peer address size, but it is not necessarily the number of bytes actually available in the caller-provided buffer. On Linux/glibc, for example, calling the library accept() with a 1-byte sockaddr buffer for an IPv4 connection returns addrlen == 16. I also see the same behaviour for getpeername(), getsockname(), and recvfrom().

So I don't think PerlSock_accept_cloexec() is the right layer to clamp this. It is a thin wrapper around accept() / accept4(), and preserving the OS/libc value-result semantics is useful because it lets callers detect truncation. The memory-safety problem is at the copy-out site: pp_accept must not copy more bytes from namebuf than were originally supplied to accept().

I agree that this does not solve silent address truncation. If the OS truncated the address into the supplied buffer, then Perl cannot recover the lost bytes by clamping. Returning an error on truncation, or trying a follow-up getpeername() on the accepted fd, would be a behaviour change and also looks non-trivial to make portable, especially for AF_UNIX where implementations differ quite a lot.

I also agree that MAXPATHLEN is a bit weird semantically here. However, changing the buffer type/size is a separate question from the memory-safety issue. MAXPATHLEN is currently a large historical buffer and may be larger than struct sockaddr_storage on some platforms, which may matter for Unix-domain pathname cases. For this PR, I think the minimal safe fix is: remember the originally supplied sockaddr buffer length and clamp the returned length to that value before copying it into a Perl scalar.

I'll update the PR to clamp against the originally supplied length rather than directly against sizeof namebuf, because the existing VMS/QNX special cases pass a smaller initial length than sizeof namebuf.

[crystarm]

On sockaddr_storage: I'm hesitant to switch pp_accept from MAXPATHLEN to struct sockaddr_storage in this PR.

sockaddr_storage is the right abstraction for “large enough for standard socket address families”, and pp_getpeername / pp_getsockname already use it where available. But MAXPATHLEN is currently larger on many systems, and AF_UNIX pathname handling is one of the places where portability gets strange. There is also prior history here: #17761 / #17764 fixed a real AIX getpeername() issue caused by a too-small fixed buffer.

So I think these are two separate concerns:

1. avoid copying past the buffer we actually supplied to the syscall --- this PR;
2. decide what buffer size/type Perl should use for socket addresses --- possible follow-up/design discussion.

Even if we later change the buffer type, the clamp is still needed defensively because the socket APIs allow the returned length to exceed the supplied buffer length.

[crystarm]

@Leont
On Linux/glibc, the library calls expose the same value-result behaviour:

• accept() with a 1-byte sockaddr buffer for an IPv4 connection returns addrlen == 16;
• getpeername() with a 1-byte sockaddr buffer returns addrlen == 16;
• getsockname() with a 1-byte sockaddr buffer returns addrlen == 16;
• recvfrom() with a 1-byte source-address buffer also returns addrlen == 16.

This does not prove every libc/platform behaves the same way, but it does prove that the behaviour is observable through the normal library calls at least on glibc. On platforms that instead return the stored/truncated length, the clamp is simply a no-op.

[crystarm]

@tonycoz
Thanks, that matches what I found.

I also reproduced this with the library calls on Linux/glibc. With an intentionally tiny sockaddr buffer, accept(), getpeername(), getsockname(), and recvfrom() all return a length larger than the supplied buffer.

You're right that this is not limited to accept(). Looking through pp_sys.c, the same class of issue appears relevant anywhere Perl passes a fixed sockaddr buffer to a socket API and then uses the returned length to expose bytes to Perl:

• pp_accept: PUSHp(namebuf, len) can read past namebuf if len is oversized;
• pp_getpeername / pp_getsockname: SvCUR_set(sv, len); *SvEND(sv) = '\0'; becomes unsafe if the returned len exceeds the allocated SV buffer;
• pp_recv / recvfrom: sv_setpvn(TARG, namebuf, bufsize) can read past namebuf if bufsize is oversized.

[crystarm]

Updated the PR.

The fix now clamps returned socket address lengths against the originally supplied buffer length, rather than against sizeof namebuf. This matters for the existing platform-specific cases where the supplied length can be smaller than namebuf.

I also covered the same copy-out pattern in getpeername() / getsockname() and recvfrom(), since those APIs can expose the same value-result length behaviour.

This remains a memory-safety fix only: it does not attempt to change Perl’s behaviour on truncated socket addresses.

Validation:

• perl Porting/updateAUTHORS.pl --validate: PASS
• git diff --check: PASS
• make test_harness: PASS

Full test result:

All tests successful.
Files=2934, Tests=1379902, 599 wallclock secs (32.58 usr  6.31 sys + 315.34 cusr 38.48 csys = 392.71 CPU)
Result: PASS
Finished test run at Thu May 21 16:52:51 2026.

[tonycoz]

We're C99 now, you can declare namesize here.

[tonycoz]

Make declaration part of the initialization.

We haven't typically gone through old code to use C99 mixed declarations and code, but updates can certainly use them.

[tonycoz]

LGTM except for the "written like it's 1990" bit, which is pretty minor 😄

[crystarm]

@tonycoz
Done! Updated declaration/initialization style. Thanks! (´｡• ◡ •｡`)

[Leont]

I can't help wondering: should this warn?