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.

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

Misc. bug: potential segmentation fault/memory corruption in llama-server #10635

Closed
anagri opened this issue Dec 3, 2024 · 2 comments · Fixed by #10651
Closed

Misc. bug: potential segmentation fault/memory corruption in llama-server #10635

anagri opened this issue Dec 3, 2024 · 2 comments · Fixed by #10651
Labels
bug Something isn't working

Comments

@anagri
Copy link

anagri commented Dec 3, 2024

Name and Version

$ ./build/bin/llama-cli --version
version: 4242 (642330a)
built with Homebrew clang version 18.1.5 for arm64-apple-darwin23.3.0

Operating systems

Linux, Mac, Windows

Which llama.cpp modules do you know to be affected?

llama-server

Problem description & steps to reproduce

in the destructor of server_context -

    ~server_context() {
        if (ctx) {
            llama_free(ctx);
            ctx = nullptr;
        }

        if (model) {
            llama_free_model(model);
            model = nullptr;
        }

        if (model_dft) {
            llama_free_model(model_dft);
            model_dft = nullptr;
        }

        // Clear any sampling context
        for (server_slot & slot : slots) {
            common_sampler_free(slot.smpl);
            slot.smpl = nullptr;

            llama_free(slot.ctx_dft);
            slot.ctx_dft = nullptr;

            common_speculative_free(slot.spec);
            slot.spec = nullptr;

            llama_batch_free(slot.batch_spec);
        }

        llama_batch_free(batch);
    }
  1. if model_dft is not selected, the slot.spec is not allocated, hence common_speculative_free is called with a nullptr (introduced in commit 9ca2e67)
  2. if model_dft is not selected, slot.batch_spec is initialized as default, and llama_batch_free with the default value causes memory corruption (introduced in commit 10bce04)

suggested fix -

  1. check for slot.spec before calling common_speculative_free
  2. convert slot.batch_spec to pointer, check for allocation and then call llama_batch_free

kind attn: @ggerganov @slaren

First Bad Commit

first bad commit: 9ca2e67
second bad commit: 10bce04

Relevant log output

this is observation from reading code, not able to reproduce with built binary and passing signal to get the system memory logs on deallocation.
@slaren
Copy link
Collaborator

slaren commented Dec 4, 2024

Agree, this is likely to be an issue. batch_spec should be initialized in the same way server_context::batch is.

@slaren slaren added bug Something isn't working and removed bug-unconfirmed labels Dec 4, 2024
@ggerganov
Copy link
Owner

PTAL #10651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants