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.

Sign up for GitHub

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

Jump to bottom

Free dynamically allocated strings #5

Closed
wants to merge 1 commit into from
Closed

Free dynamically allocated strings #5

wants to merge 1 commit into from

Conversation

eaftan
Copy link

@eaftan eaftan commented Nov 17, 2023

I believe that safe_asprintf dynamically allocates memory for the first argument, but that is not freed after the fetchers are done with them.

dspinellis
dspinellis reviewed Nov 17, 2023
View reviewed changes
Copy link
Owner

@dspinellis dspinellis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two variables are initialized once when a program starts, but used multiple times: every time a user submits a prompt. Therefore freeing them after fetching a prompt response is wrong, because they won't be available for the next fetch.

@dspinellis
Copy link
Owner

Many thanks for the PR! As commented in the review, the proposed changes will cause use-after-free errors.

@dspinellis dspinellis closed this Nov 17, 2023
@eaftan
Copy link
Author

eaftan commented Nov 17, 2023

I see, thank you for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dspinellis dspinellis dspinellis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eaftan @dspinellis