Skip to content

Fix memory management bug in llava and server code #5491

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

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Fix memory management bug in llava and server code #5491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea74ba9
Fix memory management in llava and server code
Elbios Feb 14, 2024
098ab94
Make it cleaner by checking size in batch free wrapper
Elbios Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions examples/llava/clip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1230,8 +1230,20 @@ struct clip_image_f32 * clip_image_f32_init() {
return new clip_image_f32();
}

void clip_image_u8_free (struct clip_image_u8 * img) { delete img; }
void clip_image_u8_free(struct clip_image_u8 * img) { delete img; }
void clip_image_f32_free(struct clip_image_f32 * img) { delete img; }
void clip_image_u8_batch_free(struct clip_image_u8_batch & batch) {
if (batch.size > 0) {
delete[] batch.data;
batch.size = 0;
}
}
void clip_image_f32_batch_free(struct clip_image_f32_batch & batch) {
if (batch.size > 0) {
delete[] batch.data;
batch.size = 0;
}
}

static void build_clip_img_from_data(const stbi_uc * data, int nx, int ny, clip_image_u8 * img) {
img->nx = nx;
Expand Down Expand Up @@ -1494,11 +1506,8 @@ bool clip_image_preprocess(struct clip_ctx * ctx, const clip_image_u8 * img, cli
pad_to_square = false;
}
// free the previous res_imgs if any set
if (res_imgs.size > 0 && res_imgs.size < 100) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I removed the upper bound because there didn't seem to be any justification for it, but if there is then let me know @cmp-nct and I'll restore it

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I removed the upper bound because there didn't seem to be any justification for it, but if there is then let me know @cmp-nct and I'll restore it

The reason for the upper bound was a safety check in case the passed structure points to uninitialized memory, in that case it would almost certainly be outside that range.
So only relevant if someone uses it wrong, I'm fine either way.

Glad you spotted the double free, it's another remnant of the vector->pointer refactor.

for (size_t i = 0; i < res_imgs.size; i++) {
clip_image_f32_free(&(res_imgs.data[i]));
}
delete[] res_imgs.data;
if (res_imgs.size > 0) {
clip_image_f32_batch_free(res_imgs);
}
res_imgs.data = nullptr;
res_imgs.size = 0;
Expand Down Expand Up @@ -1650,7 +1659,8 @@ bool clip_image_preprocess(struct clip_ctx * ctx, const clip_image_u8 * img, cli

res_imgs.size = 1;
res_imgs.data = new clip_image_f32[res_imgs.size];
res_imgs.data[0] = std::move(*res);
res_imgs.data[0] = *res;
clip_image_f32_free(res);

return true;
}
Expand Down
2 changes: 2 additions & 0 deletions examples/llava/clip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ CLIP_API struct clip_image_f32 * clip_image_f32_init();

CLIP_API void clip_image_u8_free (struct clip_image_u8 * img);
CLIP_API void clip_image_f32_free(struct clip_image_f32 * img);
CLIP_API void clip_image_u8_batch_free (struct clip_image_u8_batch & batch);
CLIP_API void clip_image_f32_batch_free(struct clip_image_f32_batch & batch);

CLIP_API bool clip_image_load_from_file(const char * fname, struct clip_image_u8 * img);

Expand Down
11 changes: 9 additions & 2 deletions examples/server/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,12 @@ struct llama_server_context
{
LOG_TEE("Error processing the given image");
clip_free(clp_ctx);
clip_image_f32_free(img_res_v.data);
clip_image_f32_batch_free(img_res_v);
return false;
}
if (img_res_v.size == 0)
{
LOG_TEE("Error processing the given image");
return false;
}

Expand All @@ -987,17 +992,19 @@ struct llama_server_context
if (!img.image_embedding)
{
LOG_TEE("Unable to allocate memory for image embeddings\n");
clip_image_f32_batch_free(img_res_v);
clip_free(clp_ctx);
return false;
}
LOG_TEE("slot %i - encoding image [id: %i]\n", slot.id, img.id);
if (!clip_image_encode(clp_ctx, params.n_threads, img_res, img.image_embedding))
{
LOG_TEE("Unable to encode image\n");
clip_image_f32_batch_free(img_res_v);
return false;
}

clip_image_f32_free(img_res_v.data);
clip_image_f32_batch_free(img_res_v);

img.request_encode_image = false;
}
Expand Down