Skip to content

[COPY OF #24434] pp_accept: fix potential out-of-bounds read on oversized accept() addrlen#24447

Open
crystarm wants to merge 1 commit into
Perl:bleadfrom
crystarm:fix/pp-accept-clamp-addrlen
Open

[COPY OF #24434] pp_accept: fix potential out-of-bounds read on oversized accept() addrlen#24447
crystarm wants to merge 1 commit into
Perl:bleadfrom
crystarm:fix/pp-accept-clamp-addrlen

Conversation

@crystarm

@crystarm crystarm commented May 29, 2026

Copy link
Copy Markdown

COPY OF #24434

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.

@crystarm crystarm changed the title pp_accept: fix potential out-of-bounds read on oversized accept() addrlen [COPY OF THE #24434] pp_accept: fix potential out-of-bounds read on oversized accept() addrlen May 29, 2026
@crystarm crystarm changed the title [COPY OF THE #24434] pp_accept: fix potential out-of-bounds read on oversized accept() addrlen [COPY OF #24434] pp_accept: fix potential out-of-bounds read on oversized accept() addrlen May 29, 2026
@ap ap added defer-next-dev This PR should not be merged yet, but await the next development cycle LLM contribution This PR was (believed to be) made using LLMs labels Jun 1, 2026
@ap

ap commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Readers note that a number of relevant comments in #24434 when it was closed which now still need addressing. (GitHub will not allow re-opening that PR, unfortunately, otherwise we would have done so; instead it is on readers here to take note of the comments there.)

@tonycoz

tonycoz commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

There's two issues I see:

  • leont suggested this should warn if the address is truncated, and I think that is reasonable.

  • you followed my suggestion to combine the declaration and initialization, but I was wrong to do so for pp_getpeername since it broke C++ builds, so the namesize declaration needs to be above the goto nutz, pretty much where you originally had it. Sorry about that.

@crystarm crystarm force-pushed the fix/pp-accept-clamp-addrlen branch from 3e22b26 to f6a5a48 Compare June 9, 2026 11:53
@crystarm

crystarm commented Jun 9, 2026

Copy link
Copy Markdown
Author

I added a W io warning when socket address lengths are truncated (accept, recvfrom, getpeername/getsockname), moved namesize in pp_getpeername above early goto paths for C++ build compatibility, and documented the new warning in pod/perldiag.pod.

Comment thread pp_sys.c
* supplied buffer. */
if (len > namesize) {
ck_warner(packWARN(WARN_IO),
"Socket address truncated in %s()", opname);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to setup opname like this, you can use OP_NAME(PL_op) to get the operator name.

@crystarm crystarm force-pushed the fix/pp-accept-clamp-addrlen branch from f6a5a48 to d12c210 Compare June 16, 2026 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

defer-next-dev This PR should not be merged yet, but await the next development cycle LLM contribution This PR was (believed to be) made using LLMs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants