Use of strtok() in dpdk code

Use of strtok() in dpdk code

[Isaac Boukris]

Hello,

I was debugging a crash resulting from strtok() returning NULL
unexpectedly (string still had tokens and delimiters), and the only
explanation I could come up with was that strtok is thread-unsafe and
another thread could have been calling it at the same time, and so I
changed it to use strtok_r().

That said, the only other possible use of strtok() that I could find
was in the dpdk code (telemetry), which brings me to my question,
should we consider changing all occurrences to strtok_r() or am I
missing something? there seem to be quite some in non-initialization
code.

Thanks!

Re: Use of strtok() in dpdk code

[Stephen Hemminger]

On Mon, 21 Oct 2024 21:30:02 +0300
Isaac Boukris <iboukris@gmail.com> wrote:

> Hello,
> 
> I was debugging a crash resulting from strtok() returning NULL
> unexpectedly (string still had tokens and delimiters), and the only
> explanation I could come up with was that strtok is thread-unsafe and
> another thread could have been calling it at the same time, and so I
> changed it to use strtok_r().
> 
> That said, the only other possible use of strtok() that I could find
> was in the dpdk code (telemetry), which brings me to my question,
> should we consider changing all occurrences to strtok_r() or am I
> missing something? there seem to be quite some in non-initialization
> code.
> 
> Thanks!


Most of the uses are in tests and other single threaded code.
In general, simpler just to use strtok_r everywhere and not worry about it.
Similar to not using sprintf() and instead using snprintf().

Some code scanners like codeql also flag this.

Re: Use of strtok() in dpdk code

[fengchengwen]

On 2024/10/22 9:08, Stephen Hemminger wrote:
> On Mon, 21 Oct 2024 21:30:02 +0300
> Isaac Boukris <iboukris@gmail.com> wrote:
> 
>> Hello,
>>
>> I was debugging a crash resulting from strtok() returning NULL
>> unexpectedly (string still had tokens and delimiters), and the only
>> explanation I could come up with was that strtok is thread-unsafe and
>> another thread could have been calling it at the same time, and so I
>> changed it to use strtok_r().
>>
>> That said, the only other possible use of strtok() that I could find
>> was in the dpdk code (telemetry), which brings me to my question,
>> should we consider changing all occurrences to strtok_r() or am I
>> missing something? there seem to be quite some in non-initialization
>> code.
>>
>> Thanks!
> 
> 
> Most of the uses are in tests and other single threaded code.
> In general, simpler just to use strtok_r everywhere and not worry about it.
> Similar to not using sprintf() and instead using snprintf().

I'm afraid I can't agree.

DPDK is just a SDK, it's not an application (although DPDK provided simple examples).
Many code will developped based on DPDK, we can't predict how it was implemented.
So there maybe a DPDK thread and a application thread both invoke strtok().

From this point of view, I hope that DPDK solves some of the reentrant problems of
such C functions (e.g. strtok()\strerror()).

Actually, we've try to solve before, but unfortunately it wasn't merged.
1\ strtok():   https://inbox.dpdk.org/dev/20231114110006.91148-1-haijie1@huawei.com/T/#u
2\ strerror(): https://inbox.dpdk.org/dev/20231114123552.398072-1-huangdengdui@huawei.com/T/#u

> 
> Some code scanners like codeql also flag this.

Re: Use of strtok() in dpdk code

[Isaac Boukris]

Hi

On Tue, Oct 22, 2024 at 9:51 AM fengchengwen <fengchengwen@huawei.com> wrote:
>
> DPDK is just a SDK, it's not an application (although DPDK provided simple examples).
> Many code will developped based on DPDK, we can't predict how it was implemented.
> So there maybe a DPDK thread and a application thread both invoke strtok().

I think even dpdk threads such as the telemetry ones might invoke (the
same) strtok() at the same time.

> From this point of view, I hope that DPDK solves some of the reentrant problems of
> such C functions (e.g. strtok()\strerror()).
>
> Actually, we've try to solve before, but unfortunately it wasn't merged.
> 1\ strtok():   https://inbox.dpdk.org/dev/20231114110006.91148-1-haijie1@huawei.com/T/#u
> 2\ strerror(): https://inbox.dpdk.org/dev/20231114123552.398072-1-huangdengdui@huawei.com/T/#u

+1

Re: Use of strtok() in dpdk code

[Stephen Hemminger]

On Tue, 22 Oct 2024 14:51:39 +0800
fengchengwen <fengchengwen@huawei.com> wrote:

> On 2024/10/22 9:08, Stephen Hemminger wrote:
> > On Mon, 21 Oct 2024 21:30:02 +0300
> > Isaac Boukris <iboukris@gmail.com> wrote:
> >   
> >> Hello,
> >>
> >> I was debugging a crash resulting from strtok() returning NULL
> >> unexpectedly (string still had tokens and delimiters), and the only
> >> explanation I could come up with was that strtok is thread-unsafe and
> >> another thread could have been calling it at the same time, and so I
> >> changed it to use strtok_r().
> >>
> >> That said, the only other possible use of strtok() that I could find
> >> was in the dpdk code (telemetry), which brings me to my question,
> >> should we consider changing all occurrences to strtok_r() or am I
> >> missing something? there seem to be quite some in non-initialization
> >> code.
> >>
> >> Thanks!  
> > 
> > 
> > Most of the uses are in tests and other single threaded code.
> > In general, simpler just to use strtok_r everywhere and not worry about it.
> > Similar to not using sprintf() and instead using snprintf().  
> 
> I'm afraid I can't agree.
> 
> DPDK is just a SDK, it's not an application (although DPDK provided simple examples).
> Many code will developped based on DPDK, we can't predict how it was implemented.
> So there maybe a DPDK thread and a application thread both invoke strtok().
> 
> From this point of view, I hope that DPDK solves some of the reentrant problems of
> such C functions (e.g. strtok()\strerror()).
> 
> Actually, we've try to solve before, but unfortunately it wasn't merged.
> 1\ strtok():   https://inbox.dpdk.org/dev/20231114110006.91148-1-haijie1@huawei.com/T/#u
> 2\ strerror(): https://inbox.dpdk.org/dev/20231114123552.398072-1-huangdengdui@huawei.com/T/#u
> 
> > 
> > Some code scanners like codeql also flag this.  
> 

The usages of strtok() and strerror() in init code are fine. Stuff only called from
eal_init() like devargs should be safe, but fixing it makes sense.

Perhaps a coccinelle script could help here.