Hello!
On Tue, Sep 27, 2016 at 11:23:00PM +0200, Jan Seda wrote:
> Struct sockaddr was allocated without zeroing. As getsockname() does not
> necessarily fill all the bytes, this left potentially uninitialized
> memory, which caused me a weird failure in the on-the-fly upgrade
> mechanism, where the new master failed to pair the config file AF_UNIX
> sockets with the inherited ones. ngx_cmp_sockaddr() would return
> NGX_DECLINED because the inherited one would contain uninitialized
> garbage after the actual name. This in turn would cause nginx to try to
> bind() the unix socket again, and obviously fail, because the listening
> socket was bound already (and inherited).
>
> Easy to reproduce on debian wheezy, debian jessie and CentOS 7 systems
> (all x86_64) with statically linked openssl 1.1.0. The attached patch
> seems to fix the problem.
>
> Also harmonized socklen calculation for AF_UNIX sockets to the way
> ngx_parse_unix_domain_url() does it (sizeof(struct sockaddr_un)), so
> ngx_cmp_sockaddr() is now less dangerous.
Thanks for catching this.
Dropping the length returned by getsockname() doesn't look like
a correct solution though. Instead, teaching ngx_cmp_sockaddr() to
compare sockaddrs with not-completely-filled sun_path - that is,
respecting socklen - should be the right way to go.
Please try the following patch:
diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
--- a/src/core/ngx_inet.c
+++ b/src/core/ngx_inet.c
@@ -1364,6 +1364,7 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, s
struct sockaddr_in6 *sin61, *sin62;
#endif
#if (NGX_HAVE_UNIX_DOMAIN)
+ size_t len;
struct sockaddr_un *saun1, *saun2;
#endif
@@ -1393,15 +1394,45 @@ ngx_cmp_sockaddr(struct sockaddr *sa1, s
#if (NGX_HAVE_UNIX_DOMAIN)
case AF_UNIX:
- /* TODO length */
-
saun1 = (struct sockaddr_un *) sa1;
saun2 = (struct sockaddr_un *) sa2;
- if (ngx_memcmp(&saun1->sun_path, &saun2->sun_path,
- sizeof(saun1->sun_path))
- != 0)
- {
+ if (slen1 <= (socklen_t) offsetof(struct sockaddr_un, sun_path)) {
+ return NGX_DECLINED;
+ }
+
+ if (slen2 <= (socklen_t) offsetof(struct sockaddr_un, sun_path)) {
+ return NGX_DECLINED;
+ }
+
+ if (saun1->sun_path[0] == '\0' || saun2->sun_path[0] == '\0') {
+
+ /* an abstract socket address */
+
+ if (ngx_memn2cmp((u_char *) saun1->sun_path,
+ (u_char *) saun2->sun_path,
+ slen1 - offsetof(struct sockaddr_un, sun_path),
+ slen2 - offsetof(struct sockaddr_un, sun_path))
+ != 0)
+ {
+ return NGX_DECLINED;
+ }
+
+ break;
+ }
+
+ if (slen1 < slen2) {
+ len = slen1 - offsetof(struct sockaddr_un, sun_path);
+
+ } else {
+ len = slen2 - offsetof(struct sockaddr_un, sun_path);
+ }
+
+ if (len > sizeof(saun1->sun_path)) {
+ len = sizeof(saun1->sun_path);
+ }
+
+ if (ngx_strncmp(saun1->sun_path, saun2->sun_path, len) != 0) {
return NGX_DECLINED;
}
--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
