[CRIU] [PATCH v2 2/2] unix: don't drop the path on unix sockets if they don't exist

[Tycho Andersen]

On Mon, Jul 04, 2016 at 08:38:42PM +0300, Pavel Emelyanov wrote:
> 
> > @@ -962,8 +959,53 @@ static int bind_unix_sk(int sk, struct unix_sk_info *ui)
> >  		ret = bind(sk, (struct sockaddr *)&addr,
> >  				sizeof(addr.sun_family) + ui->ue->name.len);
> >  		if (ret < 0) {
> > -			pr_perror("Can't bind socket");
> > -			goto done;
> > +			if (ui->ue->has_deleted && ui->ue->deleted && errno == EADDRINUSE) {
> > +				char temp[PATH_MAX];
> > +
> > +				pr_info("found duplicate unix socket bound at %s\n", addr.sun_path);
> > +
> > +				ret = snprintf(temp, sizeof(temp), "%s-%s-%d", addr.sun_path, "criu-temp", getpid());
> > +				/* this shouldn't happen, since sun_addr is only 108 chars long */
> > +				if (ret < 0 || ret >= sizeof(temp)) {
> > +					pr_err("snprintf of %s failed?\n", addr.sun_path);
> > +					goto done;
> > +				}
> > +
> > +				ret = rename(addr.sun_path, temp);
> > +				if (ret < 0) {
> > +					pr_perror("couldn't move socket for binding");
> > +					goto done;
> > +				}
> > +
> > +				ret = bind(sk, (struct sockaddr *)&addr,
> > +						sizeof(addr.sun_family) + ui->ue->name.len);
> > +				if (ret < 0) {
> > +					pr_perror("Can't bind socket after move");
> > +					goto done;
> > +				}
> > +
> > +				ret = unlink(addr.sun_path);
> 
> This unlink seems redundant, the rename below should unlink it.

Ok, I'll drop it.

> > +				if (ret < 0) {
> > +					pr_perror("Can't unlink socket after bind");
> > +					goto done;
> > +				}
> > +
> > +				ret = rename(temp, addr.sun_path);
> > +				if (ret < 0) {
> > +					pr_perror("couldn't move socket back");
> > +					goto done;
> > +				}
> > +
> > +				/* we've handled the deleted-ness of this
> > +				 * socket and we don't want to delete it later
> > +				 * since it's not /this/ socket.
> > +				 */
> > +				ui->ue->deleted = false;
> > +
> > +			} else {
> > +				pr_perror("Can't bind socket");
> > +				goto done;
> > +			}
> >  		}
> >  
> >  		if (*ui->name && ui->ue->file_perms) {
> > @@ -1225,6 +1267,11 @@ out:
> >  	if (restore_socket_opts(sk, ui->ue->opts))
> >  		return -1;
> >  
> > +	if (ui->ue->deleted && unlink((char *)ui->ue->name.data) < 0) {
> 
> I worry that we do this move/bind/unlink/move logic in bind_unix_sk(), which is
> called in several places, but remove the deleted name only here. Is this safe?

Good point. I think we can just do the unlink in bind_unix_sk() to
avoid having to do this everywhere, as it looks like once the socket
is bound we do all operations via the FD and not the path. This way we
can avoid inserting another call to unlink everywhere.

I'll resend, thanks!

Tycho

> > +		pr_perror("failed to unlink %s\n", ui->ue->name.data);
> > +		return -1;
> > +	}
> > +
> >  	return sk;
> >  }
> >  
> > diff --git a/images/sk-unix.proto b/images/sk-unix.proto
> > index aa2bcf7..75ec3bf 100644
> > --- a/images/sk-unix.proto
> > +++ b/images/sk-unix.proto
> > @@ -45,4 +45,5 @@ message unix_sk_entry {
> >  	 * Relative socket name may have prefix.
> >  	 */
> >  	optional string			name_dir	= 14;
> > +	optional bool			deleted		= 15;
> >  }
> > 
>