[CRIU] [PATCH] unix: Do not autobind unnamed unix sockets

[Kirill Tkhai]

On 16.06.2016 18:30, Pavel Emelyanov wrote:
> On 06/16/2016 05:55 PM, Kirill Tkhai wrote:
>>
>>
>> On 16.06.2016 17:10, Pavel Emelyanov wrote:
>>> On 06/15/2016 01:57 PM, Kirill Tkhai wrote:
>>>> addlen equal to sizeof(addr.sun_family) leads
>>>> to autobinding of socket.
>>>>
>>>> If it had been autobinded when we dumped it,
>>>> name.len wouldn't have been zero. So, this
>>>> binding is wrong. Fix that.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>>>> ---
>>>>  criu/sk-unix.c |    2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>>>> index c6cf672..9302d21 100644
>>>> --- a/criu/sk-unix.c
>>>> +++ b/criu/sk-unix.c
>>>> @@ -958,7 +958,7 @@ static int bind_unix_sk(int sk, struct unix_sk_info *ui)
>>>>  	if (prep_unix_sk_cwd(ui, &cwd_fd))
>>>>  		return -1;
>>>>  
>>>> -	if (bind(sk, (struct sockaddr *)&addr,
>>>> +	if (ui->ue->name.len && bind(sk, (struct sockaddr *)&addr,
>>>
>>> Few lines below there's another check for ui->ue->name.len. Would you
>>> merge them please?
>>
>> The whole below condition is about non-abstract bound sockets (*ui->name).
>> The whole mine if() is just about sockets non-zero len.
>>
>> If we merge them into single "if (ui->ue->name.len)" we'll have to do
>> subsequent if's. Also, below bracket paragraph already have subsequent if().
>> I don't think it's a good idea. If you mean something else, explain, please.
> 
> No, I mean exactly this.

unix: Do not autobind unnamed unix sockets
    
addlen equal to sizeof(addr.sun_family) leads
to autobinding of socket.

If it had been autobinded when we dumped it,
name.len wouldn't have been zero. So, this
binding is wrong. Fix that.

v2: Merge two conditional branches together
Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>

diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index c6cf672..ca6673e 100644
--- a/criu/sk-unix.c
+++ b/criu/sk-unix.c
@@ -958,32 +958,35 @@ static int bind_unix_sk(int sk, struct unix_sk_info *ui)
 	if (prep_unix_sk_cwd(ui, &cwd_fd))
 		return -1;
 
-	if (bind(sk, (struct sockaddr *)&addr,
-				sizeof(addr.sun_family) + ui->ue->name.len)) {
-		pr_perror("Can't bind socket");
-		goto done;
-	}
-
-	if (ui->ue->name.len && *ui->name && ui->ue->file_perms) {
-		FilePermsEntry *perms = ui->ue->file_perms;
-		char fname[PATH_MAX];
-
-		if (ui->ue->name.len >= sizeof(fname)) {
-			pr_err("The file name is too long\n");
+	if (ui->ue->name.len) {
+		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;
 		}
 
-		memcpy(fname, ui->name, ui->ue->name.len);
-		fname[ui->ue->name.len] = '\0';
+		if (*ui->name && ui->ue->file_perms) {
+			FilePermsEntry *perms = ui->ue->file_perms;
+			char fname[PATH_MAX];
 
-		if (fchownat(AT_FDCWD, fname, perms->uid, perms->gid, 0) == -1) {
-			pr_perror("Unable to change file owner and group");
-			goto done;
-		}
+			if (ui->ue->name.len >= sizeof(fname)) {
+				pr_err("The file name is too long\n");
+				goto done;
+			}
 
-		if (fchmodat(AT_FDCWD, fname, perms->mode, 0) == -1) {
-			pr_perror("Unable to change file mode bits");
-			goto done;
+			memcpy(fname, ui->name, ui->ue->name.len);
+			fname[ui->ue->name.len] = '\0';
+
+			if (fchownat(AT_FDCWD, fname, perms->uid, perms->gid, 0) == -1) {
+				pr_perror("Unable to change file owner and group");
+				goto done;
+			}
+
+			if (fchmodat(AT_FDCWD, fname, perms->mode, 0) == -1) {
+				pr_perror("Unable to change file mode bits");
+				goto done;
+			}
 		}
 	}