Skip to content

Fix infer issues#7103

Merged
douzzer merged 1 commit intowolfSSL:masterfrom
philljj:fix_infer_issues
Jan 2, 2024
Merged

Fix infer issues#7103
douzzer merged 1 commit intowolfSSL:masterfrom
philljj:fix_infer_issues

Conversation

@philljj
Copy link
Copy Markdown
Contributor

@philljj philljj commented Dec 28, 2023

Description

Fix UNINITIALIZED_VALUE bugs from infer testing.

Testing

Confirmed fixed with Infer rerun.

@philljj philljj self-assigned this Dec 28, 2023
@dgarske dgarske requested a review from wolfSSL-Bot December 28, 2023 18:18
@philljj
Copy link
Copy Markdown
Contributor Author

philljj commented Dec 28, 2023

Retest this please.

@philljj
Copy link
Copy Markdown
Contributor Author

philljj commented Dec 29, 2023

Retest this please

@philljj philljj assigned wolfSSL-Bot and unassigned philljj Dec 29, 2023
Copy link
Copy Markdown
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good -- just a few overheady analyzer coddlings to redo in a non-costly way.

Passes all the latest analyzers locally in the wolfssl-multi-test.sh ... super-quick-check suite, which is good news -- it's entirely possible to coddle one analyzer only to have a different analyzer report an unused value or other defect around the coddling.

Comment thread src/ssl.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This XMEMSET() incurs non-negligible overhead to address a clearly-false positive from infer. I can see how the analyzer got confused, given that wolfSSL_CTX_set_groups() is in another file (tls13.c), but we should try to find a better way to address this.

Comment thread src/ssl.c Outdated
Comment on lines 5616 to 5617
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could've stayed on one line, à la

int ret = 0, row = 0;

(just a FWIW)

Comment thread src/ssl.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in wolfSSL_CTX_set1_groups(). this is an overheady way to address a clearly-false positive.

Also, any idea why infer didn't similarly false-positive on wolfSSL_set1_groups()? Weird.

Comment thread src/ssl.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This XMEMSET() seems OK to me. It's not even immediately clear that it's a false positive, but mainly this is a rendering routine and the memset involves only 16 bytes.

Comment thread src/ssl.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another expensive memset to address a clearly-false positive, as above. in wolfSSL_CTX_set1_groups() etc.

Comment thread src/ssl.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same story, expensive op to address a false positive.

Comment thread src/wolfio.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addresses a false positive, but I can live with it, given it's only 6 bytes. Btw I probably would've resolved it by refactoring that neighborhood to convert the ASCII port to a word32 without the unnecessary copying of the ASCII.

@philljj
Copy link
Copy Markdown
Contributor Author

philljj commented Jan 2, 2024

Rebased and removed the added XMEMSET(groups and XMEMSET(_groups. We are considering the groups to be initialized by the for loop logic and so these are false positives.

@philljj philljj requested a review from douzzer January 2, 2024 19:29
Copy link
Copy Markdown
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@douzzer douzzer merged commit 461cf9e into wolfSSL:master Jan 2, 2024
@philljj philljj deleted the fix_infer_issues branch January 3, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants