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

There is a memory leak defect in line 3315 of the file /openssl/apps/cmp.c. #24335

Closed
LuMingYinDetect opened this issue May 6, 2024 · 2 comments
Labels
help wanted triaged: bug The issue/pr is/fixes a bug

Comments

@LuMingYinDetect
Copy link

1.On line 3304 of the file /openssl/apps/cmp.c, a pointer variable named req is defined. This pointer variable is allocated a block of dynamic memory through the function OSSL_CMP_ITAV_create on line 3311. When the first condition in the if statement on line 3312 evaluates to false (indicating that the dynamic memory allocation for req was successful) and the second condition evaluates to true, the program will return on line 3315. During this process, the dynamic memory area pointed to by req is neither used nor released, thus constituting a memory leak defect. See the illustration below:
https://github.com/LuMingYinDetect/openssl_defects/blob/main/openssl_17.png

2.The function OSSL_CMP_ITAV_create mentioned above is responsible for allocating a new block of dynamic memory and returning it. Specifically, in OSSL_CMP_ITAV_create, a pointer variable named itav is defined on line 162. This variable is allocated a block of dynamic memory through the function OSSL_CMP_ITAV_new on line 164 and returned on line 167. See the illustration below:
https://github.com/LuMingYinDetect/openssl_defects/blob/main/openssl_18.png

@LuMingYinDetect LuMingYinDetect added the issue: bug report The issue was opened to report a bug label May 6, 2024
@nhorman nhorman added triaged: bug The issue/pr is/fixes a bug help wanted labels May 6, 2024
@t8m t8m removed the issue: bug report The issue was opened to report a bug label May 6, 2024
@coolshrid
Copy link
Contributor

Based on the code, I don't see this as a memory leak.

  1. cmp_main allocates cmp_ctx using OSSL_CMP_CTX_new and uses this to invoke do_genm.
  2. do_genm allocates req and pushes/attaches this to ctx->genm_ITAVs.

    ctx param for do_genm referes to cmp_ctx from cmp_main

  3. cmp_main correctly deallocates cmp_ctx using OSSL_CMP_CTX_free

See code below, that shows the allocations/deallocations, with line numbers.

 . . .
 static int do_genm(OSSL_CMP_CTX *ctx)
 {
    . . .
    // Line #3311
    // 2.1: Allocated and saved to a list
    // req is dynamically allocated
    req = OSSL_CMP_ITAV_create(OBJ_nid2obj(opt_infotype), NULL);
    // Line #3312
    //  saved to an internal list ctx->genm_ITAVs
    if (req == NULL || !OSSL_CMP_CTX_push0_genm_ITAV(ctx, req)) {
        . . .
    }
    . . .
 }
 
 . . .

 int cmp_main(int argc, char **argv)
 {
    . . .
    // Line #3417
    // 1: cmp_ctx allocated
    cmp_ctx = OSSL_CMP_CTX_new(app_get0_libctx(), app_get0_propq());

    // Line #3573
    // 2: Invoked, req object is now part of the list cmp_ctx->genm_ITAVs
    ret = do_genm(cmp_ctx);
    . . .

    // Line #3633
    // 3: cmp_ctx free is invoked, that walks the list cmp_ctx->genm_ITAVs and
    //    free's all the allocated resources.
    OSSL_CMP_CTX_free(cmp_ctx);
 }

 . . .

@LuMingYinDetect
Copy link
Author

Based on the code, I don't see this as a memory leak.

  1. cmp_main allocates cmp_ctx using OSSL_CMP_CTX_new and uses this to invoke do_genm.
  2. do_genm allocates req and pushes/attaches this to ctx->genm_ITAVs.

    ctx param for do_genm referes to cmp_ctx from cmp_main

  3. cmp_main correctly deallocates cmp_ctx using OSSL_CMP_CTX_free

See code below, that shows the allocations/deallocations, with line numbers.

 . . .
 static int do_genm(OSSL_CMP_CTX *ctx)
 {
    . . .
    // Line #3311
    // 2.1: Allocated and saved to a list
    // req is dynamically allocated
    req = OSSL_CMP_ITAV_create(OBJ_nid2obj(opt_infotype), NULL);
    // Line #3312
    //  saved to an internal list ctx->genm_ITAVs
    if (req == NULL || !OSSL_CMP_CTX_push0_genm_ITAV(ctx, req)) {
        . . .
    }
    . . .
 }
 
 . . .

 int cmp_main(int argc, char **argv)
 {
    . . .
    // Line #3417
    // 1: cmp_ctx allocated
    cmp_ctx = OSSL_CMP_CTX_new(app_get0_libctx(), app_get0_propq());

    // Line #3573
    // 2: Invoked, req object is now part of the list cmp_ctx->genm_ITAVs
    ret = do_genm(cmp_ctx);
    . . .

    // Line #3633
    // 3: cmp_ctx free is invoked, that walks the list cmp_ctx->genm_ITAVs and
    //    free's all the allocated resources.
    OSSL_CMP_CTX_free(cmp_ctx);
 }

 . . .

Thank you for your patient explanation! Static analysis indeed sometimes presents false positives. Based on your description, I've found that the dynamically allocated memory area pointed to by req is added to ctx. It can be confirmed that this is a false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

No branches or pull requests

4 participants