Fix possible NULL pointer dereference.

[vitek]

This caused SEGFAULT when moving sql3 db from 1.7 to 1.9. With this fix I've managed to use old db successfully.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 72.41%. Comparing base (1b3aab9) to head (c8bb90e).
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/lib/db.c	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
- Coverage   72.43%   72.41%   -0.02%     
==========================================
  Files          34       34              
  Lines        9773     9775       +2     
==========================================
  Hits         7079     7079              
- Misses       2694     2696       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[williamcroberts]

If the object doesn't have the attribute CKA_ALLOWED_MECHANISMS, it should just be skipped. This patch would cause us to process everything no matter what, which is harmless, but not needed.

if (a) {
    CK_BYTE type = type_from_ptr(a->pValue, a->ulValueLen);
    if (type != TYPE_BYTE_INT_SEQ) {
        rv = _db_update_tobject_attrs(updb, tobj->id, tobj->attrs);
    }
}
tobject_free(tobj);
...

[vitek]

do you mean something like this?

if (a) {
    CK_BYTE type = type_from_ptr(a->pValue, a->ulValueLen);
    if (type != TYPE_BYTE_INT_SEQ) {
        rv = _db_update_tobject_attrs(updb, tobj->id, tobj->attrs);
    }
} else {
   tobject_free(tobj);
   continue;
}

or let it fail?

[williamcroberts]

No need for an else clause, as we need it to perform the tobject_free() or a memory leak will ensue. We also need to step the database so we can get the next row item to check.

[vitek]

so we shouldn't check rv in this case, right?

        if (a && rv != CKR_OK) {
            goto error;
        }

[williamcroberts]

Yep, you could also just unconditionally set rv = CKR_OK before the statement, either way is fine by me

[vitek]

thanks, fixed

[williamcroberts]

So overall the diff is fine, but you need to squash your two commits into one and improve the commit message and add Fixes: #845 and don't forget to signoff on your commit. Thanks!

[vitek]

So overall the diff is fine, but you need to squash your two commits into one and improve the commit message and add Fixes: #845 and don't forget to signoff on your commit. Thanks!

So overall the diff is fine, but you need to squash your two commits into one and improve the commit message and add Fixes: #845 and don't forget to signoff on your commit. Thanks!

done

[sergiorussia]

@williamcroberts could u take a look? the fix is critical

[sergiorussia]

also tagging some other active maintainers (https://github.com/orgs/tpm2-software/people) cuz last William's activity was on August 11.

@AndreasFuchsTPM
@pdxjohnny
@JuergenReppSIT
@wxleong

guys could you take a look?
the bug prevents eg from upgrading to Ubuntu 24.04 LTS

ps: after merging this one we could reject #849 as a duplicate

[sergiorussia]

@williamcroberts could you please also give any ETA for next release with the fix?

[williamcroberts]

I'm gonna start the release process this week.

…

On Wed, Sep 4, 2024, 07:21 Sergey ***@***.***> wrote: @williamcroberts <https://github.com/williamcroberts> could you please also give any ETA for next release with the fix? — Reply to this email directly, view it on GitHub <#858 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHNWB5HDVFPOK2YSZBR2SLZU33LJAVCNFSM6AAAAABEZROBW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRYHAZTGMRSGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>