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

[Gabriel Ivăncescu]

Hi Huw,

Thanks for the review.

On 14/10/2021 10:02, Huw Davies wrote:
> 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.
> 

Yeah I just realized how confusing it can be. :-)

>> @@ -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?
> 

Do you mean I should use them directly from the in-> buffer, or 
something else? If it's that, I wasn't exactly sure how the buffer 
worked, I see the output is on the same buffer and overrides it, so I 
wanted to be cautious.

Thanks,
Gabriel