Skip to content

Fix x509-store for illumos#124856

Open
gwr wants to merge 2 commits intodotnet:mainfrom
gwr:il-x509-store
Open

Fix x509-store for illumos#124856
gwr wants to merge 2 commits intodotnet:mainfrom
gwr:il-x509-store

Conversation

@gwr
Copy link
Contributor

@gwr gwr commented Feb 25, 2026

Can't use sizof(d->d_name)
Use NAME_MAX or MAXNAMLEN

Without this, the code running on illumos reads only 1 character of the file names in the certificates store and cannot find any certificates.

Can't use sizof(d->d_name)
Use NAME_MAX or MAXNAMLEN
Copilot AI review requested due to automatic review settings February 25, 2026 13:21
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 25, 2026
@gwr
Copy link
Contributor Author

gwr commented Feb 25, 2026

This is a prerequisite for using System.Net.Security on illumos.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates native X509 store enumeration to avoid relying on sizeof(dirent.d_name) (which is not usable on illumos) when allocating buffers and handling certificate file names.

Changes:

  • Compute the temporary path buffer size using NAME_MAX / MAXNAMLEN (with a fallback) instead of sizeof(d_name).
  • Add a guard to skip .pfx entries whose names won’t fit in the remaining buffer space.
  • Switch filename length computation from strnlen(..., sizeof(d_name)) to strlen(...).

@vcsjones vcsjones requested a review from bartonjs February 25, 2026 13:27
}

// On some platforms (like illumos), d_name is declared as char[1], so sizeof(d_name) doesn't give
// the actual maximum length. Use NAME_MAX or MAXNAMLEN instead.
Copy link
Member

Choose a reason for hiding this comment

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

The comment mentions MAXNAMLEN, but the code doesn't. AFAICT, MAXNAMLEN is a BSD-only name, but that BSD also defines the POSIX NAME_MAX, so MAXNAMLEN should just be removed from the comment.

// On some platforms (like illumos), d_name is declared as char[1], so sizeof(d_name) doesn't give
// the actual maximum length. Use NAME_MAX or MAXNAMLEN instead.
#ifdef NAME_MAX
static size_t maxNameLen = NAME_MAX;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a static rather than a #define? Given the current construction it looks like it could be a define...

#ifdef NAME_MAX
static size_t maxNameLen = NAME_MAX;
#else
static size_t maxNameLen = 255; // Reasonable default
Copy link
Member

Choose a reason for hiding this comment

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

What sort of system would not define it? On that system, what is going to happen if there's no null terminator?

I think we'd be better off just using NAME_MAX, and putting in this file something like

#ifndef NAME_MAX
#error NAME_MAX is not defined, an alternative method of determining the maximum file name length is required for this platform.
#endif

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Security community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants