[v2,01/15] files: Fix find_unused_fd() overflow

Submitted by Kirill Tkhai on May 27, 2016, 1:05 p.m.

Details

Message ID 146435434127.31234.15209021128423559533.stgit@pro
State Rejected
Series "Support for packet's msg_name in receive queue of promiscous DGRAM sockets"
Headers show

Commit Message

Kirill Tkhai May 27, 2016, 1:05 p.m.
This function may catch overflow near INT_MAX, so
it becomes return strange fd, like fd = -2147483648.
Fix that.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 criu/files.c         |   31 +++++++++++++++++++++++++++++++
 criu/include/files.h |    8 +-------
 2 files changed, 32 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files.c b/criu/files.c
index 16bc74d..a52e12f 100644
--- a/criu/files.c
+++ b/criu/files.c
@@ -97,6 +97,37 @@  static inline struct file_desc *find_file_desc(FdinfoEntry *fe)
 	return find_file_desc_raw(fe->type, fe->id);
 }
 
+unsigned int find_unused_fd(struct list_head *head, int hint_fd)
+{
+	struct fdinfo_list_entry *fle;
+	int fd, prev_fd;
+
+	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd))) {
+		fd = hint_fd;
+		goto out;
+	}
+	/* Return last used fd +1 */
+	fd = list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd;
+
+	if (likely(fd < INT_MAX)) {
+		fd++;
+		goto out;
+	}
+	prev_fd = INT_MAX;
+
+	list_for_each_entry_reverse(fle, head, used_list) {
+		fd = fle->fe->fd;
+		if (prev_fd > fd) {
+			fd++;
+			goto out;
+		}
+		prev_fd = fd - 1;
+	}
+	BUG();
+out:
+	return fd;
+}
+
 /*
  * A file may be shared between several file descriptors. E.g
  * when doing a fork() every fd of a forker and respective fds
diff --git a/criu/include/files.h b/criu/include/files.h
index f89164e..d46b3cd 100644
--- a/criu/include/files.h
+++ b/criu/include/files.h
@@ -137,13 +137,7 @@  static inline bool fd_is_used(struct list_head *head, int fd)
 	return false;
 }
 
-static inline unsigned int find_unused_fd(struct list_head *head, int hint_fd)
-{
-	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd)))
-		return hint_fd;
-	/* Return last used fd +1 */
-	return list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd + 1;
-}
+unsigned int find_unused_fd(struct list_head *head, int hint_fd);
 
 struct file_desc {
 	u32			id;		/* File id, unique */

Comments

Pavel Emelyanov May 30, 2016, 11:32 a.m.
On 05/27/2016 04:05 PM, Kirill Tkhai wrote:
> This function may catch overflow near INT_MAX, so
> it becomes return strange fd, like fd = -2147483648.
> Fix that.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  criu/files.c         |   31 +++++++++++++++++++++++++++++++
>  criu/include/files.h |    8 +-------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/criu/files.c b/criu/files.c
> index 16bc74d..a52e12f 100644
> --- a/criu/files.c
> +++ b/criu/files.c
> @@ -97,6 +97,37 @@ static inline struct file_desc *find_file_desc(FdinfoEntry *fe)
>  	return find_file_desc_raw(fe->type, fe->id);
>  }
>  
> +unsigned int find_unused_fd(struct list_head *head, int hint_fd)
> +{
> +	struct fdinfo_list_entry *fle;
> +	int fd, prev_fd;
> +
> +	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd))) {
> +		fd = hint_fd;
> +		goto out;
> +	}
> +	/* Return last used fd +1 */
> +	fd = list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd;
> +
> +	if (likely(fd < INT_MAX)) {
> +		fd++;

Is INT_MAX a valid file descriptor?

> +		goto out;
> +	}
> +	prev_fd = INT_MAX;
> +
> +	list_for_each_entry_reverse(fle, head, used_list) {

What's the point int list_entry(head->prev,...) above if we're scanning the
list. Can you make this code have only list_for_each_entry_reverse?

> +		fd = fle->fe->fd;
> +		if (prev_fd > fd) {
> +			fd++;
> +			goto out;
> +		}
> +		prev_fd = fd - 1;
> +	}
> +	BUG();
> +out:
> +	return fd;
> +}
> +
>  /*
>   * A file may be shared between several file descriptors. E.g
>   * when doing a fork() every fd of a forker and respective fds
> diff --git a/criu/include/files.h b/criu/include/files.h
> index f89164e..d46b3cd 100644
> --- a/criu/include/files.h
> +++ b/criu/include/files.h
> @@ -137,13 +137,7 @@ static inline bool fd_is_used(struct list_head *head, int fd)
>  	return false;
>  }
>  
> -static inline unsigned int find_unused_fd(struct list_head *head, int hint_fd)
> -{
> -	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd)))
> -		return hint_fd;
> -	/* Return last used fd +1 */
> -	return list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd + 1;
> -}
> +unsigned int find_unused_fd(struct list_head *head, int hint_fd);
>  
>  struct file_desc {
>  	u32			id;		/* File id, unique */
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Kirill Tkhai May 30, 2016, 12:13 p.m.
On 30.05.2016 14:32, Pavel Emelyanov wrote:
> On 05/27/2016 04:05 PM, Kirill Tkhai wrote:
>> This function may catch overflow near INT_MAX, so
>> it becomes return strange fd, like fd = -2147483648.
>> Fix that.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  criu/files.c         |   31 +++++++++++++++++++++++++++++++
>>  criu/include/files.h |    8 +-------
>>  2 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/criu/files.c b/criu/files.c
>> index 16bc74d..a52e12f 100644
>> --- a/criu/files.c
>> +++ b/criu/files.c
>> @@ -97,6 +97,37 @@ static inline struct file_desc *find_file_desc(FdinfoEntry *fe)
>>  	return find_file_desc_raw(fe->type, fe->id);
>>  }
>>  
>> +unsigned int find_unused_fd(struct list_head *head, int hint_fd)
>> +{
>> +	struct fdinfo_list_entry *fle;
>> +	int fd, prev_fd;
>> +
>> +	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd))) {
>> +		fd = hint_fd;
>> +		goto out;
>> +	}
>> +	/* Return last used fd +1 */
>> +	fd = list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd;
>> +
>> +	if (likely(fd < INT_MAX)) {
>> +		fd++;
> 
> Is INT_MAX a valid file descriptor?

Theoretical. I suppose, CRIU sets prlimit before a restore to make available maximum
file descriptor for a system. Otherwise, restore may fail.

The maximum fd number is hardcoded, and it's

sysctl_nr_open_max = min((size_t)INT_MAX, ~(size_t)0/sizeof(void *)) &
                             -BITS_PER_LONG;

Are you OK if I use this value instead?

>> +		goto out;
>> +	}
>> +	prev_fd = INT_MAX;
>> +
>> +	list_for_each_entry_reverse(fle, head, used_list) {
> 
> What's the point int list_entry(head->prev,...) above if we're scanning the
> list. Can you make this code have only list_for_each_entry_reverse?

Yeah, good idea.

>> +		fd = fle->fe->fd;
>> +		if (prev_fd > fd) {
>> +			fd++;
>> +			goto out;
>> +		}
>> +		prev_fd = fd - 1;
>> +	}
>> +	BUG();
>> +out:
>> +	return fd;
>> +}
>> +
>>  /*
>>   * A file may be shared between several file descriptors. E.g
>>   * when doing a fork() every fd of a forker and respective fds
>> diff --git a/criu/include/files.h b/criu/include/files.h
>> index f89164e..d46b3cd 100644
>> --- a/criu/include/files.h
>> +++ b/criu/include/files.h
>> @@ -137,13 +137,7 @@ static inline bool fd_is_used(struct list_head *head, int fd)
>>  	return false;
>>  }
>>  
>> -static inline unsigned int find_unused_fd(struct list_head *head, int hint_fd)
>> -{
>> -	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd)))
>> -		return hint_fd;
>> -	/* Return last used fd +1 */
>> -	return list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd + 1;
>> -}
>> +unsigned int find_unused_fd(struct list_head *head, int hint_fd);
>>  
>>  struct file_desc {
>>  	u32			id;		/* File id, unique */
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>
>
Pavel Emelyanov May 31, 2016, 10:14 a.m.
On 05/30/2016 03:13 PM, Kirill Tkhai wrote:
> 
> 
> On 30.05.2016 14:32, Pavel Emelyanov wrote:
>> On 05/27/2016 04:05 PM, Kirill Tkhai wrote:
>>> This function may catch overflow near INT_MAX, so
>>> it becomes return strange fd, like fd = -2147483648.
>>> Fix that.
>>>
>>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>>> ---
>>>  criu/files.c         |   31 +++++++++++++++++++++++++++++++
>>>  criu/include/files.h |    8 +-------
>>>  2 files changed, 32 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/criu/files.c b/criu/files.c
>>> index 16bc74d..a52e12f 100644
>>> --- a/criu/files.c
>>> +++ b/criu/files.c
>>> @@ -97,6 +97,37 @@ static inline struct file_desc *find_file_desc(FdinfoEntry *fe)
>>>  	return find_file_desc_raw(fe->type, fe->id);
>>>  }
>>>  
>>> +unsigned int find_unused_fd(struct list_head *head, int hint_fd)
>>> +{
>>> +	struct fdinfo_list_entry *fle;
>>> +	int fd, prev_fd;
>>> +
>>> +	if ((hint_fd >= 0) && (!fd_is_used(head, hint_fd))) {
>>> +		fd = hint_fd;
>>> +		goto out;
>>> +	}
>>> +	/* Return last used fd +1 */
>>> +	fd = list_entry(head->prev, typeof(struct fdinfo_list_entry), used_list)->fe->fd;
>>> +
>>> +	if (likely(fd < INT_MAX)) {
>>> +		fd++;
>>
>> Is INT_MAX a valid file descriptor?
> 
> Theoretical. I suppose, CRIU sets prlimit before a restore to make available maximum
> file descriptor for a system. Otherwise, restore may fail.
> 
> The maximum fd number is hardcoded, and it's
> 
> sysctl_nr_open_max = min((size_t)INT_MAX, ~(size_t)0/sizeof(void *)) &
>                              -BITS_PER_LONG;
> 
> Are you OK if I use this value instead?

We already have some value of maximum fd used by service fds. Let's use it.