Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add OPENSSL_NO_UNIX_SOCK define to exclude unix sockets support #18256

Closed
wants to merge 1 commit into from
Closed

Add OPENSSL_NO_UNIX_SOCK define to exclude unix sockets support #18256

wants to merge 1 commit into from

Conversation

maxbachmann
Copy link
Contributor

This replaces the usage of #ifdef AF_UNIX in bio/* with #ifndef NO_SYS_UN_H. This is already set for the systems we currently undefine AF_UNIX on.

Checklist

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 5, 2022
@t8m
Copy link
Member

t8m commented May 6, 2022

I am not sure this is a right fix, at least it is no straightforwardly clear

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label May 6, 2022
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 6, 2022
@maxbachmann
Copy link
Contributor Author

maxbachmann commented May 6, 2022 •
edited

My assumption was that since we do not include <sys/un.h> on the same platforms:

openssl/include/internal/sockets.h

Lines 19 to 25 in a381897

# ifdef WIN32
# define NO_SYS_UN_H
# endif
# ifdef OPENSSL_SYS_VMS
# define NO_SYS_PARAM_H
# define NO_SYS_UN_H
# endif

where <sys/un.h> only defines sockaddr_un. So currently we set NO_SYS_UN_H to prevent the inclusion of <sys/un.h> and then later undef AF_UNIX to prevent us from using sockaddr_un. I think it makes sense to use the same define to decide whether we include this file and use it's symbols afterwards.

@t8m
Copy link
Member

t8m commented May 6, 2022

Hmm, but there might be other systems that do not define AF_UNIX but are not covered by the cases where we define NO_SYS_UN_H in the sockets.h.

Is this PR fixing a concrete issue or what is the purpose?

@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label May 6, 2022
@maxbachmann
Copy link
Contributor Author

maxbachmann commented May 6, 2022 •
edited

Hmm, but there might be other systems that do not define AF_UNIX but are not covered by the cases where we define NO_SYS_UN_H in the sockets.h.

Is there any reason to import <sys/un.h when AF_UNIX is not defined?

Is this PR fixing a concrete issue or what is the purpose?

I am working on an OS, which defines AF_UNIX, but does not support unix sockets. Currently this would require me to patch the source files, while with this change I can directly pass NO_SYS_UN_H into my build system.

Another way would be to add something like OPENSSL_NO_UNIX_SOCK, which gets defined on OPENSSL_SYS_WINDOWS and OPENSSL_SYS_VMS.

@t8m
Copy link
Member

t8m commented May 6, 2022

Is there any reason to import <sys/un.h when AF_UNIX is not defined?

You've misunderstood. There is no reason for that, but what I am saying is that the NO_SYS_UN_H is defined in sockets.h only in two particular cases but there might be other targets that weren't defining AF_UNIX and your PR would break these.

Another way would be to add something like OPENSSL_NO_UNIX_SOCKETS, which gets defined on OPENSSL_SYS_WINDOWS and OPENSSL_SYS_VMS.

OK, then do it. Put it at the end of sockets.h and also define it when AF_UNIX is not defined. (Also do not forget to check whether OPENSSL_NO_UNIX_SOCKETS is already defined.)

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels May 6, 2022
maxbachmann
maxbachmann commented May 6, 2022
View reviewed changes
include/internal/sockets.h Outdated
@@ -75,7 +75,7 @@ struct servent *PASCAL getservbyname(const char *, const char *);
# include <inet.h>
# else
# include <sys/socket.h>
# ifndef NO_SYS_UN_H
# if (!defined NO_SYS_UN_H) && (defined AF_UNIX) && (!defined OPENSSL_NO_UNIX_SOCK)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this test as well, so it is enough to set OPENSSL_NO_UNIX_SOCK to disable the unix socket support

Copy link
Member

Choose a reason for hiding this comment

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

Please use the same style of using parentheses here as in other cases in this source file. !defined(NO_SYS_UN_H) &&...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated them accordingly

t8m
t8m requested changes May 6, 2022
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Just minor nits.

include/internal/sockets.h Outdated
@@ -75,7 +75,7 @@ struct servent *PASCAL getservbyname(const char *, const char *);
# include <inet.h>
# else
# include <sys/socket.h>
# ifndef NO_SYS_UN_H
# if (!defined NO_SYS_UN_H) && (defined AF_UNIX) && (!defined OPENSSL_NO_UNIX_SOCK)
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same style of using parentheses here as in other cases in this source file. !defined(NO_SYS_UN_H) &&...

include/internal/sockets.h Outdated Show resolved Hide resolved
@t8m t8m added the approval: review pending This pull request needs review by a committer label May 6, 2022
@t8m t8m changed the title Exclude unix sockets using NO_SYS_UN_H Add OPENSSL_NO_UNIX_SOCK define to exclude unix sockets support May 6, 2022
@rsbeckerca
Copy link
Contributor

This appears to build correctly on NonStop. However, other independent issues are preventing the build from completing.

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jul 7, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jul 8, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@paulidale
Copy link
Contributor

Merged, thanks for the contribution.

@paulidale paulidale closed this Jul 8, 2022
openssl-machine pushed a commit that referenced this pull request Jul 8, 2022
@maxbachmann @paulidale
Add config option OPENSSL_NO_UNIX_SOCK
081f348 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18256)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@maxbachmann @sftcd
Add config option OPENSSL_NO_UNIX_SOCK
e4e5763 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#18256)
t8m pushed a commit to t8m/openssl that referenced this pull request Nov 14, 2022
@maxbachmann @t8m
Add config option OPENSSL_NO_UNIX_SOCK
241c7f0 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#18256)

(cherry picked from commit 081f348)
openssl-machine pushed a commit that referenced this pull request Nov 21, 2022
@maxbachmann @t8m
Add config option OPENSSL_NO_UNIX_SOCK
dc6daea 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18256)

(cherry picked from commit 081f348)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maxbachmann @t8m @rsbeckerca @openssl-machine @paulidale