Skip to content

Check for potential null pointer#61

Open
SafeCoding233 wants to merge 2 commits into
apache:trunkfrom
SafeCoding233:trunk
Open

Check for potential null pointer#61
SafeCoding233 wants to merge 2 commits into
apache:trunkfrom
SafeCoding233:trunk

Conversation

@SafeCoding233

@SafeCoding233 SafeCoding233 commented Sep 20, 2024

Copy link
Copy Markdown

apr_palloc may return NULL pointer. It may trigger undefined behavior in later usage. For example, passing a null pointer to memcpy may trigger undefined behavior. Thus it is better to check for NULL.

@notroj

notroj commented Oct 1, 2024

Copy link
Copy Markdown
Contributor

With APR pools, the accepted coding style is to assume apr_palloc() never fails - applications can and install an abort function using apr_pool_create_ex() which is invoked if the underlying memory allocation ever fails.

@SafeCoding233

SafeCoding233 commented Oct 1, 2024

Copy link
Copy Markdown
Author

With APR pools, the accepted coding style is to assume apr_palloc() never fails - applications can and install an abort function using apr_pool_create_ex() which is invoked if the underlying memory allocation ever fails.

Thank you for your kind reply and explanation!
I also thought the assumption is apr_palloc() never fails. However, I made the PR because I found some code snippets in the repo that indeed check for the possible null pointers.

For example:

(*key) = (apr_threadkey_t *)apr_palloc(pool, sizeof(apr_threadkey_t));
if ((*key) == NULL) {
return APR_ENOMEM;

apr/jose/apr_jose.c

Lines 71 to 73 in 4283c6a

jose->jose.json = apr_palloc(pool, sizeof(apr_jose_json_t));
if (!jose->jose.json) {
return NULL;

So I thought it might be good to follow a consistent assumption?

I must admit that I didn't have experience contributing to the APR repo before. My apologies if my understanding is inaccurate, and thank you again for your time!

@notroj

notroj commented Feb 14, 2025

Copy link
Copy Markdown
Contributor

You're right that not all APR code follows the convention, unfortunately.

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.

3 participants