Re: [iproute2 2/2] ss: fix NULL pointer access when parsing unix sockets with oldformat

[Antonio Quartulli <a@unstable.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2160 bytes --]

Hi,

On 07/01/18 03:28, Stefano Brivio wrote:
> On Sun,  7 Jan 2018 02:31:50 +0800
> Antonio Quartulli <a@unstable.cc> wrote:
> 
>> When parsing and printing the unix sockets in unix_show(),
>> if the oldformat is detected, the peer_name member of the sockstat
>> object is left uninitialized (NULL).
> 
> Luckily, it is initialized. I'd rather say:
> 
> 	[...]
> 	object is not set (NULL).

ops, you are right! "is not set" makes more sense.

> 
>> For this reason, if a filter has been specified on the command line,
>> a strcmp() will crash when trying to access it.
>>
>> Avoid crash by checking that peer_name is not NULL before
>> passing it to strcmp().
>>
>> Cc: Stefano Brivio <sbrivio@redhat.com>
>> Cc: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> 
> Fixes: 2d0e538f3e1c ("ss: Drop list traversal from unix_stats_print()")
> 
>> [...]
>>
>> diff --git a/misc/ss.c b/misc/ss.c
>> index b35859dc..29a25070 100644
>> --- a/misc/ss.c
>> +++ b/misc/ss.c
>> @@ -3711,7 +3711,10 @@ static int unix_show(struct filter *f)
>>  			};
>>  
>>  			memcpy(st.local.data, &u->name, sizeof(u->name));
>> -			if (strcmp(u->peer_name, "*"))
>> +			/* when parsing the old format rport is set to 0 and
>> +			 * therefore peer_name remains NULL
>> +			 */
> 
> Maybe this comment is a bit redundant, but I don't have a strong
> preference either.
> 

I thought explaining "why" it could be NULL would help the casual reader
understand why we do check it.
But now that we have the check, I think it's quick to understand why we
need it.

I'd leave to whoever is going to merge the match to decide to keep or
not the comment.

>> +			if (u->peer_name && strcmp(u->peer_name, "*"))
>>  				memcpy(st.remote.data, &u->peer_name,
>>  				       sizeof(u->peer_name));
>>  			if (run_ssfilter(f->f, &st) == 0) {
> 
> FWIW:
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> 


Thanks a lot, Stefano!

Should I send v2 with the commit message changed? Or can we leave this
to who will merge the change?


Regards,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]