Fix deref after null #17293

Goblenus · 2021-12-16T12:52:22Z

ctx may be NULL at 178 line

ctx may be NULL at 178 line CLA: trivial

t8m · 2021-12-16T13:53:56Z

providers/implementations/storemgmt/file_store.c

-    file_close(ctx);
+    if (ctx != NULL)
+        file_close(ctx);
+


There is actually no point in calling file_close(ctx) here. Just call free_file_ctx(ctx) which works with NULL argument.

t8m · 2021-12-16T13:54:38Z

providers/implementations/storemgmt/file_store.c

+    if (loaderctx == NULL)
+        assert(0);
+


I do not think we want such assert calls. Please drop this.

t8m

I am OK with CLA: trivial

mattcaswell · 2021-12-16T16:27:03Z

providers/implementations/storemgmt/file_store.c

@@ -190,7 +190,7 @@ static void *file_open_dir(const char *path, const char *uri, void *provctx)
    }
    return ctx;
 err:
-    file_close(ctx);
+    free_file_ctx(ctx);


Is this really the correct behaviour? We can end up here if OPENSSL_DIR_read returns a NULL value and errno != 0. If that occurs but OPENSSL_DIR_read still populated ctx->_.dir.ctx then file_close still seems like the correct thing to do. Looking at LPdir_unix.c it seems that this could actually happen if the readdir call here returns NULL:

openssl/crypto/LPdir_unix.c

Lines 129 to 132 in dd2fcc1

direntry = readdir((*ctx)->dir);

if (direntry == NULL) {

return 0;

}

Ah, you're right.

Return file_close back?

So the correct fix is to just return NULL instead of the first goto err; in the function.

No problem. Need some time. Do my way home.

incorrect

Goblenus · 2021-12-17T07:16:06Z

@t8m Sorry for that long delay. Can you verify it once more :) Done my best this time :)

t8m · 2021-12-17T07:24:28Z

LGTM

Goblenus · 2021-12-17T07:34:21Z

@t8m Should I need to set "Resolved" all outdated code discussions or you'll done it by yourself?

t8m · 2021-12-17T08:03:11Z

@t8m Should I need to set "Resolved" all outdated code discussions or you'll done it by yourself?

No need for that.

mattcaswell · 2021-12-17T10:51:47Z

I agree this is trivial

openssl-machine · 2021-12-18T11:00:12Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

paulidale · 2021-12-19T23:54:51Z

Merged to master and 3.0. Thanks for the fix.

ctx may be NULL at 178 line CLA: trivial Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17293)

ctx may be NULL at 178 line CLA: trivial Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #17293) (cherry picked from commit 68b78dd)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Dec 16, 2021

Fix deref after null

9c62261

ctx may be NULL at 178 line CLA: trivial

Goblenus force-pushed the fix_deref_after_null branch from 209aee6 to 9c62261 Compare December 16, 2021 12:59

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Dec 16, 2021

t8m requested changes Dec 16, 2021

View reviewed changes

t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: bug The issue/pr is/fixes a bug labels Dec 16, 2021

Drop assert and file_close change to free_file_ctx

54850de

Goblenus requested a review from t8m December 16, 2021 14:08

t8m previously approved these changes Dec 16, 2021

View reviewed changes

t8m added the approval: review pending This pull request needs review by a committer label Dec 16, 2021

mattcaswell reviewed Dec 16, 2021

View reviewed changes

t8m self-requested a review December 16, 2021 16:38

Set early exit

cb1552c

Goblenus requested a review from mattcaswell December 17, 2021 07:16

t8m approved these changes Dec 17, 2021

View reviewed changes

mattcaswell approved these changes Dec 17, 2021

View reviewed changes

mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Dec 17, 2021

paulidale approved these changes Dec 19, 2021

View reviewed changes

paulidale closed this Dec 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deref after null #17293

Fix deref after null #17293

Goblenus commented Dec 16, 2021

t8m Dec 16, 2021

Goblenus Dec 16, 2021

t8m Dec 16, 2021

Goblenus Dec 16, 2021

t8m left a comment

mattcaswell Dec 16, 2021

t8m Dec 16, 2021

Goblenus Dec 16, 2021

t8m Dec 16, 2021

Goblenus Dec 16, 2021

Goblenus commented Dec 17, 2021 •

edited

t8m commented Dec 17, 2021

Goblenus commented Dec 17, 2021 •

edited

t8m commented Dec 17, 2021

mattcaswell commented Dec 17, 2021

openssl-machine commented Dec 18, 2021

paulidale commented Dec 19, 2021

	direntry = readdir((*ctx)->dir);
	if (direntry == NULL) {
	return 0;
	}

Fix deref after null #17293

Fix deref after null #17293

Conversation

Goblenus commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t8m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Goblenus commented Dec 17, 2021 • edited

t8m commented Dec 17, 2021

Goblenus commented Dec 17, 2021 • edited

t8m commented Dec 17, 2021

mattcaswell commented Dec 17, 2021

openssl-machine commented Dec 18, 2021

paulidale commented Dec 19, 2021

Goblenus commented Dec 17, 2021 •

edited

Goblenus commented Dec 17, 2021 •

edited