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

Fix an unused variable issue in nxt_term_parse() as found by clang-analyzer and coverity #1441

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Sep 23, 2024

The following changes since commit ba234b4db2d137b7dcf63188cc19e0493ebbc01a:

  Version bump (2024-09-17 22:42:09 +0100)

are available in the Git repository at:

  [email protected]:ac000/unit.git ca-fix

for you to fetch changes up to bba84a73a259b7cb93e3c0a52a0aa7253e53042c:

  src/test: Add an extra test case to nxt_term_parse_test.c (2024-09-24 14:55:04 +0100)

----------------------------------------------------------------

The first patch fixes a clang-analyzer and coverity issue where we assign
a value and then assign again without using it inbetween.

The second patch adds a test case for strings with trailing whitespace.

----------------------------------------------------------------
Andrew Clayton (2):
      Resolve unused assignment in nxt_term_parse()
      src/test: Add an extra test case to nxt_term_parse_test.c

 src/nxt_time_parse.c           | 24 +++++++++---------------
 src/test/nxt_term_parse_test.c |  1 +
 2 files changed, 10 insertions(+), 15 deletions(-)

Copy link
Contributor

@hongzhidao hongzhidao left a comment

Choose a reason for hiding this comment

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

Great job.
I feel the first commit looks good and sufficient, and the subsequent changes can be discarded.

src/nxt_time_parse.c Outdated Show resolved Hide resolved
src/nxt_time_parse.c Outdated Show resolved Hide resolved
@ac000 ac000 changed the title Do some cleanups in nxt_term_parse() (fixing a clang-analyzer (and coverity) issue) Fix an unused variable issue in nxt_term_parse() as found by clang-analyzer and coverity Sep 24, 2024
@ac000
Copy link
Member Author

ac000 commented Sep 24, 2024

  • Drop patches 2 & 3
  • Add a patch that adds an extra test case
$ git range-diff d2603475...bba84a73
1:  b401e6b6 < -:  -------- Rename st_space to st_next in nxt_term_parse()
2:  d2603475 < -:  -------- Remove some dead code from nxt_term_parse()
-:  -------- > 1:  bba84a73 src/test: Add an extra test case to nxt_term_parse_test.c

@ac000
Copy link
Member Author

ac000 commented Sep 24, 2024

Thanks @hongzhidao !

Both clang-analyzer and coverity flagged an issue in nxt_term_parse()
that we set 'state = st_letter' but then set it to 'state = st_space'
before using it.

While we could simply remove the first assignment and placate the
analyzers, upon further analysis it seems that there is some more
cleanup that could be done in this function.

This commit addresses the above issue, subsequent commits will continue
the cleanup.

To solve the unused assignment issue we can get rid of the

  'state == st_letter'

assignment and unconditionally execute the code that was behind the

  if (state != st_letter) {

guard. If we're not handling a space then we should have either a digit
or letter.

Also, perhaps more importantly, this if () statement would never be
false at this point as state would never == st_letter.

We may as well also remove the st_letter enum value.

The src/test/nxt_term_parse_test.c still passes

  tests: [notice] term parse test passed

NOTE: Although this function is not currently used in Unit (only by
src/test/nxt_term_parse_test.c), it is probably worth cleaning it up and
solving one of the open clang-analyzer (and coverity) issues.

Signed-off-by: Andrew Clayton <[email protected]>
The function nxt_term_parse() is able to take strings with trailing
whitespace e.g. "1w1d ", add a test case to cover such things.

Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Sep 24, 2024

Rebased with master

$ git range-diff bba84a73...a9d687e7
-:  -------- > 1:  355f038f Compile with -funsigned-char
1:  06a7f650 = 2:  b358e7fb Resolve unused assignment in nxt_term_parse()
2:  bba84a73 = 3:  a9d687e7 src/test: Add an extra test case to nxt_term_parse_test.c

@ac000 ac000 merged commit a9d687e into nginx:master Sep 24, 2024
24 checks passed
@ac000 ac000 deleted the ca-fix branch September 24, 2024 15:02
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.

2 participants