Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C] report by Clang Static Analyzer #7221

Open
wing328 opened this issue Aug 15, 2020 · 13 comments
Open

[C] report by Clang Static Analyzer #7221

wing328 opened this issue Aug 15, 2020 · 13 comments

Comments

@wing328
Copy link
Member

wing328 commented Aug 15, 2020

I used Clang Static Analyzer to detect potential issues with the client and got a report like the following:

Screenshot 2020-08-15 at 7 03 40 PM

Attached is the full report (unzip and open index.html in the browser).

scan-build-report.zip

We'll review and address these issues with separate PRs.

If anyone wants to help, please reply to let us know.

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

@auto-labeler
Copy link

auto-labeler bot commented Aug 15, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@ityuhui
Copy link
Contributor

ityuhui commented Aug 15, 2020

Hi @wing328 @zhemant @michelealbano

I'd like to address some fixes.
To avoid duplicated work, I'll pick up the defects in src/apiClient.c firstly

Bug Group Bug Type ▾ File Function/Method Line Path Length Fixed by PR 
API Argument with 'nonnull' attribute passed null src/apiClient.c apiClient_invoke 275 12 #7285
Dead store Dead assignment src/apiClient.c assembleTargetUrl 146 1 #7255
Memory error Memory leak src/apiClient.c apiClient_invoke 257 18 #7285

@ityuhui
Copy link
Contributor

ityuhui commented Aug 25, 2020

Hi @wing328 @zhemant @michelealbano

I reviewed all cases of memory leak in model/*.c and think they are false alarms.

Take the below case as an example:

Bug Group Bug Type ▾ File Function/Method Line Path Length -
Memory error Memory leak model/pet.c pet_parseFromJSON 244 30  
    pet_local_var = pet_create ( //←  Potential memory leak
        id ? id->valuedouble : 0,
        category ? category_local_nonprim : NULL,
        strdup(name->valuestring),
        photo_urlsList,
        tags ? tagsList : NULL,
        status ? statusVariable : -1
        );

    return pet_local_var;   //<-- pet_local_var is returned and may memory-leak

The openapi c client only allocates the memory of model (e.g. pet), returns to user to use.
It's not the resonsibility of openapi c client to free the memory. Actually, users should free the memory in their source code after the model is not needed:

e.g. (in unit-tests/manual-PetAPI.c)

    pet_t *pet = pet_create(EXAMPLE_PET_ID,
		           category,
		           petName,
		           photoUrls,
		           tags,
		           status);
    ...
    pet_free(pet);  // <-- free the memory here.

@ityuhui
Copy link
Contributor

ityuhui commented Aug 26, 2020

I'm going to address the remaining bugs in api/*.c

I think 2 PR will be enought.

one is for

Bug Group Bug Type ▾ File Function/Method Line Path Length  Fixed by PR
Memory error Memory leak api/PetAPI.c PetAPI_uploadFile 705 5 #7302

And the other one is for

Bug Group Bug Type ▾ File Function/Method Line Path Length  Fixed by PR
Logic error Uninitialized argument value api/PetAPI.c PetAPI_updatePetWithForm 597 13 #7305

@ityuhui
Copy link
Contributor

ityuhui commented Aug 28, 2020

There is only one problem remaining:

Bug Group Bug Type ▾ File Function/Method Line Path Length  
Logic error Uninitialized argument value api/PetAPI.c PetAPI_deletePet 190 6

But I do not understand why the localVarToReplace_petId is marked as "Uninitialized argument value"

Hi @wing328 @zhemant @michelealbano

Do you have any comments about this problem ?

@ityuhui
Copy link
Contributor

ityuhui commented Sep 1, 2020

Hi @wing328

Since there is no more question and comment, I think the bugs reported by this ticket are resolved.
Could you please re-run the Clang Static Analyzer and verify my PR ?

@wing328
Copy link
Member Author

wing328 commented Sep 2, 2020

sep2report.zip

Attached please find the updated report.

@ityuhui
Copy link
Contributor

ityuhui commented Sep 2, 2020

Thank you @wing328

4 new problems are reported by this time.
I think 1~2 PR will fix them.

Bug Group Bug Type ▾ File Function/Method Line Path Length  Fixed by PR
Logic error Uninitialized argument value api/PetAPI.c PetAPI_uploadFile 718 17 #7332
Logic error Uninitialized argument value api/PetAPI.c PetAPI_deletePet 198 13 #7332
Logic error Uninitialized argument value api/PetAPI.c PetAPI_updatePetWithForm 610 15 #7332
Logic error Uninitialized argument value api/PetAPI.c PetAPI_updatePetWithForm 616 17 #7332

@ityuhui
Copy link
Contributor

ityuhui commented Sep 5, 2020

Thanks @wing328

Could you please re-run the scan of Clang Static Analyzer again ?

@wing328
Copy link
Member Author

wing328 commented Sep 6, 2020

Attached please find the latest report

sep6report.zip

Only 2 issues remaining 👍

@ityuhui
Copy link
Contributor

ityuhui commented Sep 7, 2020

Hi @wing328

The detail of remaining issues are described above, I think they are false alarm. So there is no issues reported by CSA scan actually now.

There is only one problem remaining:

Bug Group Bug Type ▾ File Function/Method Line Path Length  
Logic error Uninitialized argument value api/PetAPI.c PetAPI_deletePet 190 6
But I do not understand why the localVarToReplace_petId is marked as "Uninitialized argument value"

Hi @wing328 @zhemant @michelealbano

Do you have any comments about this problem ?

@wing328
Copy link
Member Author

wing328 commented Dec 16, 2020

Attached please find the new report created a few minutes ago.
dec16report.zip

@ityuhui
Copy link
Contributor

ityuhui commented Dec 17, 2020

There is no new issue found by the dec16report.

The 2 issues are false-alarm we have analyzed at above comments.

Attached please find the new report created a few minutes ago.
dec16report.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants