[CRIU] [PATCHv2] x86/crtools: Fix null pointer dereference

[Dmitry Safonov]

On 5/2/19 10:36 AM, Radostin Stoyanov wrote:
> Dereferencing a null pointer is undefined behavior.
> 
> ISO/IEC 9899, clause 6.5.3.2, paragraph 4
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf

That doesn't make sense, sorry.

sizeof() operator doesn't evaluate expression as long as it's not a
var-array (which is not the case), check in the paper 6.5.3.4:
"If the type of the operand is a variable length array type, the operand
is evaluated; otherwise, the operand is not evaluated and the result is
an integer constant."

Basically, in this case it's a compile-time constant.
I.e.:
sizeof(valid_xsave_frame(NULL)) will be the same as sizeof(bool),
without actual runtime function call.

So, as I don't see any point in the change,

Nacked-by: Dmitry Safonov <0x7f454c46 at gmail.com>

Though, there are those ugly casts to (long) which could be addressed
with %zu instead of %li modifier.

> 
> v2: declare x with always unused attribute. (thanks Cyrill Gorcunov)
> 
> Signed-off-by: Radostin Stoyanov <rstoyanov1 at gmail.com>
> ---
>  criu/arch/x86/crtools.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
> index ee016da00..865efc513 100644
> --- a/criu/arch/x86/crtools.c
> +++ b/criu/arch/x86/crtools.c
> @@ -288,21 +288,21 @@ void arch_free_thread_info(CoreEntry *core)
>  static bool valid_xsave_frame(CoreEntry *core)
>  {
>  	UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
> -	struct xsave_struct *x = NULL;
> +	struct xsave_struct __always_unused x;
>  
> -	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x->i387.st_space)) {
> +	if (core->thread_info->fpregs->n_st_space < ARRAY_SIZE(x.i387.st_space)) {
>  		pr_err("Corruption in FPU st_space area "
>  		       "(got %li but %li expected)\n",
>  		       (long)core->thread_info->fpregs->n_st_space,
> -		       (long)ARRAY_SIZE(x->i387.st_space));
> +		       (long)ARRAY_SIZE(x.i387.st_space));
>  		return false;
>  	}
>  
> -	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x->i387.xmm_space)) {
> +	if (core->thread_info->fpregs->n_xmm_space < ARRAY_SIZE(x.i387.xmm_space)) {
>  		pr_err("Corruption in FPU xmm_space area "
>  		       "(got %li but %li expected)\n",
>  		       (long)core->thread_info->fpregs->n_st_space,
> -		       (long)ARRAY_SIZE(x->i387.xmm_space));
> +		       (long)ARRAY_SIZE(x.i387.xmm_space));
>  		return false;
>  	}
>  
> 

Thanks,
          Dmitry