[BUG][C] intToStr() macro is broken

[imaami]

The generated C client code defines this macro:

#define intToStr(dst, src) \
    do {\
    char dst[256];\
    snprintf(dst, 256, "%ld", (long int)(src));\
}while(0)

This is broken because the destination array goes out of scope before it can be used. The generated code shows clearly that there is shadowing going on. This is what the result looks like after macro expansion:

~/git/openapi-generator$ cpp -P samples/client/petstore/c/api/PetAPI.c | grep -A2 localVarBuff_petId | head -3 | clang-format --assume-filename=x.c -
char localVarBuff_petId[256];
do {
	char localVarBuff_petId[256];
	snprintf(localVarBuff_petId, 256, "%ld", (long int)(petId));
} while (0);
localVarPath = strReplace(localVarPath, localVarToReplace_petId, localVarBuff_petId);

I don't know how the generated code is able to work. Probably just a lucky case of compiler optimizations hiding the undefined behavior.

[wing328]

@imaami thanks for reporting the issue

did you try the latest master? there's been a lot of fixes/enhancements recently made to the C client generator.

https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-to-test-with-the-latest-master-of-openapi-generator

[imaami]

@imaami thanks for reporting the issue

did you try the latest master? there's been a lot of fixes/enhancements recently made to the C client generator.

https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-to-test-with-the-latest-master-of-openapi-generator

I didn't specifically try it, I can see the problem is currently there and hasn't been touched in 6 years.

Definition:

openapi-generator/modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

Lines 8 to 12 in ff0fe26

	#define intToStr(dst, src) \
	do {\
	char dst[256];\
	snprintf(dst, 256, "%ld", (long int)(src));\
	}while(0)

Uses:

openapi-generator/modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

Lines 142 to 145 in ff0fe26

	char localVarBuff_{{paramName}}[256];
	intToStr(localVarBuff_{{paramName}}, {{paramName}});
	localVarPath = strReplace(localVarPath, localVarToReplace_{{paramName}}, localVarBuff_{{paramName}});

openapi-generator/modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

Lines 155 to 158 in ff0fe26

	char localVarBuff_{{paramName}}[256];
	intToStr(localVarBuff_{{paramName}}, *{{paramName}});
	localVarPath = strReplace(localVarPath, localVarToReplace_{{paramName}}, localVarBuff_{{paramName}});

openapi-generator/modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

Lines 168 to 171 in ff0fe26

	char localVarBuff_{{paramName}}[256];
	intToStr(localVarBuff_{{paramName}}, {{paramName}});
	localVarPath = strReplace(localVarPath, localVarToReplace_{{paramName}}, localVarBuff_{{paramName}});

openapi-generator/modules/openapi-generator/src/main/resources/C-libcurl/api-body.mustache

Lines 181 to 184 in ff0fe26

	char localVarBuff_{{paramName}}[256];
	intToStr(localVarBuff_{{paramName}}, {{paramName}});
	localVarPath = strReplace(localVarPath, localVarToReplace_{{paramName}}, localVarBuff_{{paramName}});

[imaami]

It would be helpful to add -Wall -Wextra to CPPFLAGS when compiling with GCC. The default warnings in GCC are inadequate by any reasonable standard.

[wing328]

thanks for the additional info.

cc @zhemant (2018/11) @ityuhui (2019/12) @michelealbano (2020/03)

also cc @eafer who contributed many PRs recently.

[wing328]

@imaami FYI .Here is one recent improvement to the CMake file: https://github.com/OpenAPITools/openapi-generator/pull/20332/files#diff-ca9e359f588565d588ea904884e08c822102d784b18234aa07553cfd214bd2e2R9

[imaami]

@imaami FYI .Here is one recent improvement to the CMake file: https://github.com/OpenAPITools/openapi-generator/pull/20332/files#diff-ca9e359f588565d588ea904884e08c822102d784b18234aa07553cfd214bd2e2R9

The CFLAGS change in that commit enables two specific warnings and makes them build errors. Enabling warnings one at a time is tedious as there are dozens of useful ones. -Wall -Wextra is more or less the reasonable minimum for GCC. There will be more warnings in the logs to tackle, almost all of them valid.

[eafer]

@wing328 Thank you for the cc, I was going to ask you to keep me in the loop for C changes.

@imaami The reason I made those three warnings into build errors is because the automated testing only runs a build, and those particular warnings match a few regressions that have affected me lately. I agree that -Wall -Wextra is a good idea, but keep in mind that the C generator is not very popular and has a lot of issues.

As for the bug you noticed: I don't think that macro should exist, snprintf() should be called directly. Do you want to send a patch yourself or do you want me to do it?

[imaami]

@wing328 Thank you for the cc, I was going to ask you to keep me in the loop for C changes.

@imaami The reason I made those three warnings into build errors is because the automated testing only runs a build, and those particular warnings match a few regressions that have affected me lately. I agree that -Wall -Wextra is a good idea, but keep in mind that the C generator is not very popular and has a lot of issues.

To be clear, I think those added -Werror flags are a good thing of course. :)

I've looked at the current state of the generated C code and you're right, it needs work. I'll try to do what I can, I could also use a good C generator for all the various LLM APIs out there. I'm cursed with being experienced in C and liking it a lot, but also being fascinated with the possibilities of AI. C and AI APIs are not a popular combination at all.

As for the bug you noticed: I don't think that macro should exist, snprintf() should be called directly. Do you want to send a patch yourself or do you want me to do it?

I'll do it now, won't take long.

[eafer]

To be clear, I think those added -Werror flags are a good thing of course. :)

Of course heh, my point was that -Wall -Wextra alone won't catch any regressions because the automated testing swallows warnings, and -Wall -Wextra -Werror may cause trouble for users with different versions of the compiler.

Also -Wmissing-declarations is not part of -Wall -Wextra.

[imaami]

To be clear, I think those added -Werror flags are a good thing of course. :)

Of course heh, my point was that -Wall -Wextra alone won't catch any regressions because the automated testing swallows warnings, and -Wall -Wextra -Werror may cause trouble for users with different versions of the compiler.

Hmm, shouldn't it be possible to enable the verbose makefile setting in cmake for automated testing?

[eafer]

No idea, I had never used cmake before.

[imaami]

Here's the commit. There's no regen of the sample code done, could someone who knows the codebase better take care of that? https://github.com/imaami/openapi-generator/tree/fix-c-int-to-string

[eafer]

I think they need you to regenerate the samples so that the ci can check that everything still builds. It's not hard to do, assuming that you already built the codebase: https://github.com/OpenAPITools/openapi-generator/blob/master/.github/PULL_REQUEST_TEMPLATE.md

[imaami]

I think they need you to regenerate the samples so that the ci can check that everything still builds. It's not hard to do, assuming that you already built the codebase: https://github.com/OpenAPITools/openapi-generator/blob/master/.github/PULL_REQUEST_TEMPLATE.md

Alright, thanks for the link. I think PR #20383 should be OK now.