[CRIU] images rework

[Pavel Emelyanov]

On 04/09/2012 04:24 PM, Cyrill Gorcunov wrote:
> On Mon, Apr 09, 2012 at 04:12:04PM +0400, Pavel Emelyanov wrote:
>> On 04/09/2012 04:10 PM, Cyrill Gorcunov wrote:
>>> On Mon, Apr 09, 2012 at 04:04:14PM +0400, Pavel Emelyanov wrote:
>>>> Hi.
>>>>
>>>> Recent changes in CRIU change images significantly.
>>>> Summary:
>>>>
>>>
>>> I suspect the patch series is upcoming, right? ;) 
>>
>> Already in the repo.
> 
> I see. Look, I don;t like what have you done with struct fd_parms
> initialization.
> 
> We've had
> 
> -	/* Dump /proc/pid/cwd */
> -	params = (struct fd_parms) {
> -		.id		= FD_ID_INVALID,
> -		.pid		= FD_PID_INVALID,
> -		.type		= FDINFO_CWD,
> -	};
> -
> -	fd = open_proc(pid, "cwd");
> -	if (fd < 0)
> -		return -1;
> 
> so when I extend fd_parms structure itself I know
> that new members will be zero here. When we init
> it as new code does
> 
> +static int dump_task_fs(pid_t pid, struct cr_fdset *fdset)
> +{
> +	struct fd_parms p;
> 	...
> +	p.type = FDINFO_REG;
> +	p.flags = 0;
> +	p.pos = 0;
> +	fe.cwd_id = fd_id_generate_special();
> +
> +	ret = dump_one_reg_file(fd, fe.cwd_id, &p);
> 
> the new members in fd_parms might becoe garbage from
> stack.
> 
> That said, it's not a problem at moment but really
> a time bomb I think. So I would suggest to patch it
> as
> 
> +static int dump_task_fs(pid_t pid, struct cr_fdset *fdset)
> +{
> +       struct fd_parms p = { };
>         ...
> +       p.type = FDINFO_REG;
> +       fe.cwd_id = fd_id_generate_special();
> 
> what do you think?

I think that you're trying to replace one bomb with another. Having a
garbage in a structure makes you sometimes hit a bug when using field you
wasn't supposed to, but putting zeroes into it can make you live with it
without realizing that you're doing smth wrong.

> 	Cyrill
> .
>