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

Clang static analyser fixes #1154

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Clang static analyser fixes #1154

merged 2 commits into from
Mar 5, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Feb 23, 2024

These commits fix a couple of issues found by the clang static analyser.

  • Var: Remove a dead assignment in nxt_var_interpreter()

The first removes a dead assignment whereby we store the return from a nxt_cpymem() bu then return from the function before doing anything further with it.

  • Avoid potential NULL pointer dereference in nxt_router_temp_conf()

The second fixes a potential NULL pointer dereference.

@ac000 ac000 marked this pull request as ready for review March 5, 2024 05:57
@ac000 ac000 requested a review from hongzhidao March 5, 2024 05:57
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.

LGTM.

And just another way.

    if (rtcf != NULL && rtcf->tstr_state != NULL) {
        nxt_tstr_state_release(rtcf->tstr_state);
    }

Both are ok, fix it in your preferred way.

@ac000
Copy link
Member Author

ac000 commented Mar 5, 2024

LGTM.

And just another way.

    if (rtcf != NULL && rtcf->tstr_state != NULL) {
        nxt_tstr_state_release(rtcf->tstr_state);
    }

Both are ok, fix it in your preferred way.

Yes, that also works. However I like the overall goto label clean up which i think improves the whole function...

p is not used again before returning from the function.

Found by the clang static analyser.

Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
In nxt_router_temp_conf() we have

  rtcf = nxt_mp_zget(mp, sizeof(nxt_router_conf_t));
  if (nxt_slow_path(rtcf == NULL)) {
      goto fail;
  }

If rtcf is NULL then we do

  fail:

  if (rtcf->tstr_state != NULL) {
      nxt_tstr_state_release(rtcf->tstr_state);
  }

In which case we will dereference the NULL pointer rtcf.

This patch re-works the goto labels to make them more specific to their
intended purpose and ensures we are freeing things which have been
allocated.

This was found by the clang static analyser.

Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
@ac000
Copy link
Member Author

ac000 commented Mar 5, 2024

  • Rebased with master
  • Added Zhidao's Reviewed-bys
$ git range-diff 532c4bab...c2f7f296
 -:  -------- >  1:  e2cab032 Remove debug from builds and tests
 -:  -------- >  2:  faa7e792 Packages: Pass CFLAGS to compile wasm modules on all packaging targets
 -:  -------- >  3:  9d02b906 Add PHP 8.2 and 8.1 to test matrix
 -:  -------- >  4:  d5665f49 Update setup-go to v5
 -:  -------- >  5:  4d25c612 Edited changes.xml for the 1.32.0 release
 -:  -------- >  6:  ace553dc Generated Dockerfiles for Unit 1.32.0
 -:  -------- >  7:  08811700 Added version 1.32.0 CHANGES
 -:  -------- >  8:  e67d7433 Version bump
 -:  -------- >  9:  23e807de Wasm-wc: use more common uname switch to get operating system name
 -:  -------- > 10:  8ff606fb Configuration: Fix check in nxt_conf_json_parse_value()
 -:  -------- > 11:  4eb008bb Remove unused nxt_vector_t API
 1:  67635d72 ! 12:  353d2d05 Var: Remove a dead assignment in nxt_var_interpreter()
    @@ Commit message
     
         Found by the clang static analyser.
     
    +    Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
     
      ## src/nxt_var.c ##
 2:  532c4bab ! 13:  c2f7f296 Avoid potential NULL pointer dereference in nxt_router_temp_conf()
    @@ Commit message
     
         This was found by the clang static analyser.
     
    +    Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
     
      ## src/nxt_router.c ##

@ac000 ac000 merged commit c2f7f29 into nginx:master Mar 5, 2024
17 checks passed
@ac000 ac000 deleted the clang-sa-fixes branch March 5, 2024 23:13
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