[PATCH 0/5] fix segment fault when parse args

[fengchengwen]

On 2023/3/21 21:50, Ferruh Yigit wrote:
> On 3/17/2023 2:43 AM, fengchengwen wrote:
>> On 2023/3/17 2:18, Ferruh Yigit wrote:
>>> On 3/14/2023 12:48 PM, Chengwen Feng wrote:
>>>> The rte_kvargs_process() was used to parse KV pairs, it also supports
>>>> to parse 'only keys' (e.g. socket_id) type. And the callback function 
>>>> parameter 'value' is NULL when parsed 'only keys'.
>>>>
>>>> It may leads to segment fault when parse args with 'only key', this 
>>>> patchset fixes rest of them.
>>>>
>>>> Chengwen Feng (5):
>>>>   app/pdump: fix segment fault when parse args
>>>>   net/memif: fix segment fault when parse devargs
>>>>   net/pcap: fix segment fault when parse devargs
>>>>   net/ring: fix segment fault when parse devargs
>>>>   net/sfc: fix segment fault when parse devargs
>>>
>>> Hi Chengwen,
>>>
>>> Did you scan all `rte_kvargs_process()` instances?
>>
>> No, I was just looking at the modules I was concerned about.
>> I looked at it briefly, and some modules had the same problem.
>>
>>>
>>>
>>> And if there would be a way to tell kvargs that a value is expected (or
>>> not) this checks could be done in kvargs layer, I think this also can be
>>> to look at.
>>
>> Yes, the way to tell kvargs may lead to a lot of modifys and also break ABI.
>> I also think about just set value = "" when only exist key, It could perfectly solve the above segment scene.
>> But it also break the API's behavior.
>>
> 
> What about having a new API, like `rte_kvargs_process_extended()`,
> 
> That gets an additional flag as parameter, which may have values like
> following to indicate if key expects a value or not:
> ARG_MAY_HAVE_VALUE  --> "key=value" OR 'key'
> ARG_WITH_VALUE      --> "key=value"
> ARG_NO_VALUE        --> 'key'
> 
> Default flag can be 'ARG_MAY_HAVE_VALUE' and it becomes same as
> `rte_kvargs_process()`.
> 
> This way instead of adding checks, relevant usage can be replaced by
> `rte_kvargs_process_extended()`, this requires similar amount of change
> but code will be more clean I think.
> 
> Do you think does this work?

Yes, it can work.

But I think the introduction of new API adds some complexity.
And a good API definition could more simpler.

> 
> 
>>
>> Or continue fix the exist code (about 10+ place more),
>> for new invoking, because the 'arg_handler_t' already well documented (52ab17efdecf935792ee1d0cb749c0dbd536c083),
>> they'll take the initiative to prevent this.
>>
>>
>> Hope for more advise for the next.
>>
>>> .
>>>
> 
> .
>