Don't use strtok(3) #276

haesbaert · 2025-07-24T14:03:59Z

strtok(3) is not re-entrant, use strtok_r(3).

--

strtok(3) will break on multithreaded programs, basically anyone using the watch callback would break on multithreaded.

k8s-ci-robot · 2025-07-24T14:04:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haesbaert
Once this PR has been reviewed and has the lgtm label, please assign brendandburns for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

linux-foundation-easycla · 2025-07-24T14:04:05Z

The committers listed above are authorized under a signed CLA.

✅ login: haesbaert / name: Christiano Haesbaert (4bd8a7d, 84552ff)

k8s-ci-robot · 2025-07-24T14:04:07Z

Welcome @haesbaert!

It looks like this is your first PR to kubernetes-client/c 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/c has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

brendandburns · 2025-07-25T19:52:07Z

kubernetes/config/authn_plugin/plugins/oidc/libkubernetes_oidc.c

 static time_t get_token_expiration_time(const char *token_string)
 {
    static char fname[] = "get_token_expiration_time()";
+    char *last;


nit: please set to explicit NULL

brendandburns · 2025-07-25T19:52:18Z

kubernetes/watch/watch_util.c


 static int wu_convert_to_json_array(list_t * json_array, const char *json_string)
 {
+    char *last;


here too wrt NULL

brendandburns · 2025-07-25T19:53:15Z

small request for explicit NULL for the last variables, otherwise LGTM.

You also need to sign the CLA.

ityuhui · 2025-07-26T01:48:02Z

strtok_r is not implemented on Windows.
Use conditional compilation to avoid build failures on Windows

For example, in this project:

#ifndef _WIN32

or

check_symbol_exists(secure_getenv "stdlib.h" HAVE_SECURE_GETENV)

#if defined(HAVE_SECURE_GETENV)

haesbaert · 2025-07-28T07:13:31Z

small request for explicit NULL for the last variables, otherwise LGTM.

You also need to sign the CLA.

Fixed the nit, I need to get approval for the CLA, will come back to you, meanwhile I fixed another bug, see below.

haesbaert · 2025-07-28T07:15:01Z

strtok_r is not implemented on Windows. Use conditional compilation to avoid build failures on Windows

For example, in this project:
#ifndef _WIN32

or

check_symbol_exists(secure_getenv "stdlib.h" HAVE_SECURE_GETENV)

#if defined(HAVE_SECURE_GETENV)

Thanks, I've added a check for HAVE_STRTOK_R, which also made me realize the test for HAVE_STRNDUP was broken as utils.c never included "config.h". I'm falling back from strtok_r to strtok, if that's not good enough, I'd suggest copying the one from OpenBSD since it's simple enough and it's BSD3 licensed.

ityuhui · 2025-07-29T13:22:42Z

Hi @haesbaert

Thanks for the PR! I recommend:

check_symbol_exists(secure_getenv "stdlib.h" HAVE_STRTOK_R)

    char *p = NULL;
    char *last = NULL;
#if defined(HAVE_STRTOK_R)
    p = strtok_r(dup_token_string, OIDC_ID_TOKEN_DELIM, &last);  /* jwt header */
#else
    p = strtok(dup_token_string, OIDC_ID_TOKEN_DELIM); /* jwt header */
#endif
    if (!p) {
        fprintf(stderr, "%s: The token <%s> is not a valid JWT token.\n", fname, token_string);
        goto end;
    }
#if defined(HAVE_STRTOK_R)
    p = strtok_r(NULL, OIDC_ID_TOKEN_DELIM, &last);  /* jwt part2 */
#else
    p = strtok(NULL, OIDC_ID_TOKEN_DELIM); /* jwt part2 */
#endif
    if (!p) {
        fprintf(stderr, "%s: The token <%s> is not a valid JWT token.\n", fname, token_string);
        goto end;

Just like HAVE_SECURE_GETENV and HAVE_GETENV do.

What do you think ?

strtok(3) is not re-entrant, use strtok_r(3), since windows doesn't have it, guard it under HAVE_STRTOK_R on the same scheme as HAVE_STRNCPY. While adding HAVE_STRTOK_R I noticed utils.c was not including config.h, which also made the HAVE_STRNCPY test fail, so fix it as well.

haesbaert · 2025-09-08T11:41:43Z

Hi @haesbaert

Thanks for the PR! I recommend:

check_symbol_exists(secure_getenv "stdlib.h" HAVE_STRTOK_R)

    char *p = NULL;
    char *last = NULL;
#if defined(HAVE_STRTOK_R)
    p = strtok_r(dup_token_string, OIDC_ID_TOKEN_DELIM, &last);  /* jwt header */
#else
    p = strtok(dup_token_string, OIDC_ID_TOKEN_DELIM); /* jwt header */
#endif
    if (!p) {
        fprintf(stderr, "%s: The token <%s> is not a valid JWT token.\n", fname, token_string);
        goto end;
    }
#if defined(HAVE_STRTOK_R)
    p = strtok_r(NULL, OIDC_ID_TOKEN_DELIM, &last);  /* jwt part2 */
#else
    p = strtok(NULL, OIDC_ID_TOKEN_DELIM); /* jwt part2 */
#endif
    if (!p) {
        fprintf(stderr, "%s: The token <%s> is not a valid JWT token.\n", fname, token_string);
        goto end;

Just like HAVE_SECURE_GETENV and HAVE_GETENV do.

What do you think ?

Thanks for getting back and apologies for the long delay, got busy and there was some holidays.
I've applied your suggestions, let me know if this is what you had in mind.

kubernetes/src/utils.c

ityuhui · 2025-09-11T10:13:59Z

kubernetes/config/authn_plugin/plugins/oidc/libkubernetes_oidc.c

@@ -1,3 +1,4 @@
+#include "config.h"


There is a build error due to this including. https://github.com/kubernetes-client/c/actions/runs/17549510255/job/50128037985?pr=276

maybe need to include the relative path of config.h or change the CMakeLists.txt in the oidc directory.

I included the src directory in 4bd8a7d
I'm not sure how to trigger a build that builds that, so I couldn't properly test.

The build still fails.

You can build locally using the following command.

cd kubernetes/config/authn_plugin/plugins/oidc mkdir build cd build cmake .. make

ityuhui · 2025-09-11T10:16:29Z

Hi @haesbaert

Welcome back ! and thanks for making these changes.
I have a few minor comments.

k8s-ci-robot requested review from brendandburns and ityuhui July 24, 2025 14:04

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2025

brendandburns reviewed Jul 25, 2025

View reviewed changes

haesbaert force-pushed the nostrtok branch from 1ff9572 to 84552ff Compare September 8, 2025 11:38

ityuhui reviewed Sep 11, 2025

View reviewed changes

kubernetes/src/utils.c Show resolved Hide resolved

ityuhui reviewed Sep 11, 2025

View reviewed changes

Add src to include path since it needs config.h

4bd8a7d

Don't use strtok(3) #276

Are you sure you want to change the base?

Don't use strtok(3) #276

Uh oh!

Conversation

haesbaert commented Jul 24, 2025

Uh oh!

k8s-ci-robot commented Jul 24, 2025

Uh oh!

linux-foundation-easycla bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 24, 2025

Uh oh!

brendandburns Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

brendandburns Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Jul 25, 2025

Uh oh!

ityuhui commented Jul 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haesbaert commented Jul 28, 2025

Uh oh!

haesbaert commented Jul 28, 2025

Uh oh!

ityuhui commented Jul 29, 2025

Uh oh!

haesbaert commented Sep 8, 2025

Uh oh!

Uh oh!

ityuhui Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

haesbaert Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

ityuhui Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

ityuhui commented Sep 11, 2025

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Jul 24, 2025 •

edited

Loading

ityuhui commented Jul 26, 2025 •

edited

Loading