-
Notifications
You must be signed in to change notification settings - Fork 13.9k
server: add support for local image path loading for server #16874
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
base: master
Are you sure you want to change the base?
server: add support for local image path loading for server #16874
Conversation
|
I also locally ran |
…cs-Inc/llama.cpp into server-local-image-loading
ngxson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I don't feel like adding this feature as it can be a huge security risk. Better to do it only when you can list out all of the edge cases and do proper testing.
tools/server/utils.hpp
Outdated
| std::string fname = url.substr(7); | ||
| std::vector<std::string> fparts = string_split<std::string>(fname, std::filesystem::path::preferred_separator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we simply do string_replace_all(url, "file://", "") ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of url.substr(7)? I thought strictly stripping off the file:// prefix was the minimal modification necessary to the potential file path prior to checking and validating it was legitimate. E.g. if for some reason it was a malformed path with file:// later on, this would also replace that. Maybe that doesn't matter since it would already be an invalid path but it could introduce a case where it leads to an unexpectedly valid path by removing this in the middle.
tools/server/utils.hpp
Outdated
| } | ||
| // load local file path | ||
| raw_buffer buf; | ||
| FILE * f = fopen(fname.c_str(), "rb"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better using std::ifstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to, but I was trying to take cues from how this was done in https://github.com/ggml-org/llama.cpp/blob/master/tools/mtmd/mtmd-helper.cpp#L439
tools/server/utils.hpp
Outdated
| } | ||
| } | ||
| // Check allowed local media path - fs_validate_filename already validated that there is no ".." or "." | ||
| if (opt.allowed_local_media_path == "" || !string_starts_with(fname, opt.allowed_local_media_path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opt.allowed_local_media_path = "/home/user";
fname = "/home/userabc"
// string_starts_with(fname, opt.allowed_local_media_path) can be true right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - in theory this would only be an issue if both /home/user and /home/userabc are valid directories, in which case it's somewhat ambiguous if setting opt.allowed_local_media_path = "/home/user" is intended to allow both dirs or strictly /home/user, but I would argue it should only allow the exact dir specified. I'll consider how best to account for this.
Co-authored-by: Xuan-Son Nguyen <thichthat@gmail.com>
Co-authored-by: Xuan-Son Nguyen <thichthat@gmail.com>
Co-authored-by: Xuan-Son Nguyen <thichthat@gmail.com>
This is my first time looking at or contributing to llama.cpp - can you point me in the right direction for where/how to add test cases in the codebase for this type of feature? I'm happy to do so, just a bit lost. I was trying to copy the style and approach as much as possible from other parts of the existing llama.cpp codebase (e.g. |
…cs-Inc/llama.cpp into server-local-image-loading
…g parsing on startup instead of when a request comes in
|
I've swapped to using I also added test cases to Please let me know if there are additional tests you'd like me to add (either for the arg parser, or the |
image_urlnow supportsfile://URIs to load images from local file paths. Similar to vLLM, this is guarded by a new parameter--allowed-local-media-path(default none), as well as a maximum file size parameter--local-media-max-size-mb(default 15mb).