Repository navigation
feat(net): store DNS servers set through sceNetSetDnsInfo and sceNetSetDns6Info - #9506
Conversation
Progress report (7a25f87 → 44393a3)📈 System libraries: 49.95% (+0.03%, +3 implemented functions) ✅ 3 implemented
Changed libraries (implemented exports)
|
MarcZX10
left a comment
There was a problem hiding this comment.
The repo's overlap rule says that when the overlap report identifies open PRs changing the same lines or exports, authors must agree which lands first and the PR must declare that order with Depends on.
I checked the current open libSceNet PRs against this branch. Pairwise merge attempts report content conflicts in Export.cpp and/or docs/dev/debt/libSceNet.md with #5836, #8204, #8225, #8232, #8234, #8235, #8254 and #8362. The DNS test registration also shares the libSceNet CMake insertion area changed by #5836/#8254 and the test reorganization in #6044. Several of the function changes are conceptually separate, but the overlap rule still requires coordinating their landing order and recording any prerequisite in the checklist.
Please contact the affected authors to agree on the order, update Depends on to list the agreed prerequisites (or none if they confirm this PR can land first), and rerun the overlap/conventions checks after updating. I’m requesting changes only for this missing coordination record; the current eligibility, conventions, intake, labels and report checks pass. Linux, Windows, relinker and sanitizer product jobs are skipped for this PR, so they provide no hosted build result here. The PR reports the Ubuntu DNS test and conventions/test-registration checks; I have not rerun a local build.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The DNS state behavior is localized and the author accurately records that console semantics are unverified, but there is a guest-pointer safety gap in the new setters. sceNetSetDnsInfo dereferences info->primaryAddress and later copies *info; sceNetSetDns6Info reads info->primaryAddress and copies *info directly. These are guest pointers, so an invalid non-null address can fault the host process instead of returning a guest error. The new tests cover null and value checks, but not unreadable non-null inputs. Please copy/probe these structures through the project’s guest-memory-safe mechanism before reading them, preserve the existing error behavior on invalid pointers, and add a regression for an unreadable non-null pointer. I did not run local tests. Separately, the required coordination/order for overlapping libSceNet export/debt/CMake/test edits is still unresolved; please record agreed predecessors or author-confirmed none before merge.
|
I reviewed the three new DNS functions and found a guest-pointer safety concern: both setters read fields through |
df8fc15 to
bed7b5c
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Correction to my earlier pointer-safety finding: the current head copies both non-null input structures through GuestSockets::ReadGuest before inspecting them, and GuestNetDnsInfo.cpp now includes unreadable-page regression cases. That resolves the host-fault concern; I withdraw it.
Two items remain before merge:
- The current overlap report lists shared libSceNet CMake, export and debt paths with multiple open PRs, including #9523, #9525, #9542, #9543 and #9544. The checklist still says
Depends on: none, and there is no author coordination recorded. Please compare the exact hunks, agree landing order with affected authors wherever ordering is required, declare prerequisites, and refresh the overlap check. - This branch and its reported Linux test are based on main
6ef5a1c4, while current upstream main isc61d20f54801efc3798555829f6fedcff71ad7ff. Please update the branch and rerun the focused DNS/pointer test, conventions and overlap checks on current main.
The current hosted eligibility, conventions, labels and report checks pass; product/relinker jobs are skipped. I did not rerun local tests. No remaining pointer-safety defect is claimed.
bed7b5c to
13d0c80
Compare
|
Overlap check for reviewers, on head 13d0c80 and current I merge-tested this branch against the 53 open PRs that touch the libSceNet No other open PR implements or changes |
harisibrar1
left a comment
There was a problem hiding this comment.
Checked against main 830ccd1 and shadPS4 net.cpp at 0918faa0255e9816b795b62c9b09eaea51a1bc5e (L1343-1400):
- The struct layouts match main's
NetDnsInfo/NetDns6Info(Export.cpp:199-213). The set/get round trip, clear-on-null and counts match the pinned shadPS4 code, and the singleg_dnsLockcovers every access. Thanks for the careful port. - Rules question on the setters: the stored servers are only read back by the two getters (
g_dnsOverride/g_dns6Overrideat Export.cpp:2393 and :2621). Every lookup still goes through host::getaddrinfo/::getnameinfo(main Export.cpp:2051-2058, :2162). SosceNetSetDnsInfo({8.8.8.8, 0}, 0)returns success, butsceNetResolverStartNtoakeeps querying the host's servers. CONTRIBUTING.md only allows a success that doesn't do the work as a silent stub that unblocks a title and only affects the UI, listed under "Silent stubs". HereUnblocksis none and the debt entry sits under "Functional". Would you consider keeping the two setters onNotImplemented_nid_no_patchuntil a title needs them (or until the resolver uses the stored servers), or else moving the entry to Silent stubs with the title it unblocks? - Small consistency point: main's
sceNetGetDnsInfothrows for nonzero flags and for a null info because the console results are unknown (Export.cpp:2370-2371). The newsceNetGetDns6InforeturnsNET_EINVALfor the same unmeasured inputs, taken from shadPS4. It might be worth handling both getters the same way, either both throwing or both following shadPS4 with the source cited in the debt entry.
Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.
13d0c80 to
e2dd1a1
Compare
|
Thanks for the careful review, @harisibrar1. Getters: agreed. Setters: storing the guest's servers and reporting them back through the getters is the behaviour this PR models. The part it doesn't reproduce, the resolver using those servers, is a gap, and the debt entry now states that gap first: the resolver keeps using the host's DNS servers, and the setters only store the guest's servers for the getters to report back. A title that sets servers and reads them back gets consistent answers, while lookups still go through the host resolver. I didn't list them under Silent stubs because the stored values are observable and tested, not a bare success. If a maintainer would rather keep both setters on Rebased onto main 9b1ed88. |
|
Firmware check of the argument checks, which follow shadPS4 here:
|
Apply the firmware 13.42 module-probe report in PR boykopovar#9506: all four DNS calls check initialization first; getters return EINVAL for null outputs or nonzero flags; setters reject flags before reading guest memory. Keep the checked input copy and document EFAULT as host safety, not firmware behavior. Use the current public Net.hpp declarations in the regression. Rebased onto upstream e7e3e9c. Ubuntu 24.04, GCC 13.3, Release: actual libc/libkernel/libSceNet libraries and both focused test executables built. guest_sce_net passes. A separate harness linked against those unmodified libraries passes initialization, errno, invalid flags, unreadable-input flag precedence, null clearing and unchanged-output checks. The full guest_net_dns_info test is blocked here: process_vm_readv is denied with EPERM by the execution environment. No full DNS regression pass is claimed. Public-header audit, conventions, test registration and whitespace checks pass. The resolver still uses host DNS; IPv6 host-server discovery and firmware server-count behavior remain debt. Firmware evidence: boykopovar#9506 (comment) AI-assisted: yes
e2dd1a1 to
9e469c2
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
Apply the firmware 13.42 module-probe report in PR boykopovar#9506: all four DNS calls check initialization first; getters return EINVAL for null outputs or nonzero flags; setters reject flags before reading guest memory. Keep the checked input copy and document EFAULT as host safety, not firmware behavior. Use the current public Net.hpp declarations in the regression. Rebased onto upstream e7e3e9c. Ubuntu 24.04, GCC 13.3, Release: actual libc/libkernel/libSceNet libraries and both focused test executables built. guest_sce_net passes. A separate harness linked against those unmodified libraries passes initialization, errno, invalid flags, unreadable-input flag precedence, null clearing and unchanged-output checks. The full guest_net_dns_info test is blocked here: process_vm_readv is denied with EPERM by the execution environment. No full DNS regression pass is claimed. Public-header audit, conventions, test registration and whitespace checks pass. The resolver still uses host DNS; IPv6 host-server discovery and firmware server-count behavior remain debt. Firmware evidence: boykopovar#9506 (comment) AI-assisted: yes
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
9e469c2 to
44393a3
Compare
|
Checked the setters' guest read ( |
What
Converts
sceNetSetDnsInfo,sceNetSetDns6InfoandsceNetGetDns6InfofromNotImplemented_nid_no_patch:sceNetSetDnsInfoandsceNetSetDns6Infocopy the info through the checked guest read (GuestSockets::ReadGuest) and store the given DNS servers; an unreadable info answersEFAULT, a null info clears them, and a zero primary server or nonzero flags answerEINVAL.sceNetGetDnsInforeports the stored IPv4 servers once some are set and the host's servers otherwise.sceNetGetDns6Inforeports the stored IPv6 servers, or none, and returns the server count; a null info or nonzero flags answerEINVALon both getters, as measured on firmware 13.42 (BunnyFl's check in this thread). Every DNS call answersENOTINITbeforesceNetInit.The resolver keeps using the host's servers: the stored servers are only reported back by the two getters, and
docs/dev/debt/libSceNet.mdrecords that gap. The argument checks and counts follow shadPS4's net.cpp; they are not measured on a console and are recorded indocs/dev/debt/libSceNet.md. libSceNet goes from 64/223 to 67/223 intools/progress.py.Tested
Windows 11 26H2 (build 10.0.26300, WinLibs GCC 15.2.0 r7, full Release build), head e2dd1a1 rebased onto main 9b1ed88:
guest_net_dns_info(IPv4 and IPv6 set, get, clear and argument checks,EFAULTwith the stored servers kept for an unreadable non-null info, andsceNetGetDns6Infothrowing for nonzero flags and a null info) andguest_sce_net(including its IPv6 socket checks) pass, 2/2.tools/check_conventions.py --base origin/mainandtools/check_test_registration.pypass.tools/pr_overlap.pyreports no shared lines or exports with other open pull requests; it lists only shared files it could not compare because those pull requests conflict withmain. An earlier Linux run (Ubuntu 24.04, GCC 13.3, Release) of 13d0c80 on main ab347d1 passedguest_net_dns_infoandguest_sce_net'ssceNetGetDnsInfochecks. No title tested.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)