Skip to content

Conversation

@ruslo
Copy link
Contributor

@ruslo ruslo commented May 23, 2024

  • documentation is added or updated

@t8m t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) tests: exempted The PR is exempt from requirements for testing branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 labels May 23, 2024
@t8m
Copy link
Member

t8m commented May 23, 2024

I am not sure about this one. Factually it is of course true, however there are many other functions like this that obviously construct some new objects either in clean state or by decoding some data. I would not like to confuse users that only these PEM_read functions create objects to be owned by caller. If we documented all the similar places then OK but just changing this place looks wrong to me.

@ruslo
Copy link
Contributor Author

ruslo commented May 23, 2024

If we documented all the similar places then OK but just changing this place looks wrong to me

I'm just clarifying parts that cover my scenario, which I tested with valgrind. I haven't touched any other API. Can you give me a list of similar places?

levitte
levitte previously approved these changes May 24, 2024
@levitte levitte dismissed their stale review May 24, 2024 07:57

Dismissing my own review, as it may have been premature

@levitte
Copy link
Member

levitte commented May 24, 2024

I'm thinking that the page describing all the i2d_ and d2i_ functions would be another candidate for this sort of amendment. Curiously, though, the i2d_ function description has a passage like this:

If *ppout is NULL memory will be allocated for a buffer and the encoded
data written to it. In this case *ppout is not incremented and it points
to the start of the data just written.

So it seems that in some places, we do at least give a hint that something needs to be freed.

- Free objects returned from PEM read
- Free objects returned from d2i_*
@ruslo ruslo force-pushed the pr.pem_read_free branch from fcde432 to 43aa3f9 Compare May 26, 2024 15:35
@ruslo
Copy link
Contributor Author

ruslo commented May 26, 2024

I'm thinking that the page describing all the i2d_ and d2i_ functions would be another candidate for this sort of amendment.

A similar note added to doc/man3/d2i_X509.pod.

Let me know what other pages you think need to be updated.

@ruslo ruslo changed the title [Docs] Free objects returned from PEM read [Docs] Notes about freeing objects May 26, 2024
@paulidale paulidale 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 May 29, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 30, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request Jun 7, 2024
- Free objects returned from PEM read
- Free objects returned from d2i_*

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #24478)
@t8m
Copy link
Member

t8m commented Jun 7, 2024

Merged to all the active branches. Thank you for your contribution.

@t8m t8m closed this Jun 7, 2024
openssl-machine pushed a commit that referenced this pull request Jun 7, 2024
- Free objects returned from PEM read
- Free objects returned from d2i_*

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #24478)

(cherry picked from commit d4700c0)
openssl-machine pushed a commit that referenced this pull request Jun 7, 2024
- Free objects returned from PEM read
- Free objects returned from d2i_*

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #24478)

(cherry picked from commit d4700c0)
openssl-machine pushed a commit that referenced this pull request Jun 7, 2024
- Free objects returned from PEM read
- Free objects returned from d2i_*

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #24478)

(cherry picked from commit d4700c0)
openssl-machine pushed a commit that referenced this pull request Jun 7, 2024
- Free objects returned from PEM read
- Free objects returned from d2i_*

Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #24478)

(cherry picked from commit d4700c0)
@ruslo ruslo deleted the pr.pem_read_free branch June 7, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.0 Applies to openssl-3.0 branch branch: 3.1 Applies to openssl-3.1 (EOL) branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants