Re: [PATCH v3 1/3] nsiproxy: Convert the reply to ICMP_ECHO_REPLY in the output buffer.

[Huw Davies]

On Mon, Oct 11, 2021 at 06:14:38PM +0300, Gabriel Ivăncescu wrote:

I haven't looked at this properly, but here are a few comments.

The patch subject could do with some work.
> @@ -4658,17 +4630,15 @@ DWORD WINAPI IcmpSendEcho2Ex( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc_r
>      opt_size = opts ? (opts->OptionsSize + 3) & ~3 : 0;
>      in_size = FIELD_OFFSET(struct nsiproxy_icmp_echo, data[opt_size + request_size]);
>      in = heap_alloc_zero( in_size );
> -
>
> out_size = reply_size - sizeof(ICMP_ECHO_REPLY) + sizeof(*out);
> -
>
> out = heap_alloc( out_size );
> -
>
> if (!in || !out)
> - if (!in)
> {
> -
>    heap_free( out );
> -
>    heap_free( in );
>    SetLastError( IP_NO_RESOURCES );
>    return 0;
> }
> - in->addr = (ULONG_PTR)reply;

In the context of sending an icmp message, "addr" is an extremely poor
name choice!  Something like "user_reply_ptr" would be better.
> @@ -320,19 +372,34 @@ static DWORD WINAPI listen_thread_proc( void *arg )
>  {
>      IRP *irp = arg;
>      IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
> - unsigned int reply_len = irpsp->Parameters.DeviceIoControl.OutputBufferLength;
> struct nsiproxy_icmp_echo *in = irp->AssociatedIrp.SystemBuffer;
> struct icmp_listen_params params;
> - ULONG family = in->dst.si_family;
> - ULONGLONG addr = in->addr;
> - ULONG bits = in->bits;

Why do "family", "bits" and "addr" need separate variables?

Please drop patch [3/3].

Huw.