Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
10 changes: 9 additions & 1 deletion ggml/include/gguf.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,18 @@ extern "C" {
struct ggml_context ** ctx;
};

// reads up to `len` bytes at `offset` into `output` and returns the number of bytes read.
// may be called with `len == 0` to seek/synchronize to `offset` without reading.
// when `len == 0` returns 0 on success and non-zero on failure
typedef size_t (*gguf_reader_callback_t)(void * userdata, uint8_t * output, size_t offset, size_t len);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// reads up to `len` bytes at `offset` into `output` and returns the number of bytes read.
// may be called with `len == 0` to seek/synchronize to `offset` without reading.
// when `len == 0` returns 0 on success and non-zero on failure
typedef size_t (*gguf_reader_callback_t)(void * userdata, uint8_t * output, size_t offset, size_t len);
// callback to simulate or wrap a FILE pointer:
// - by default, read up to `len` bytes at `offset` into `output` and return the number of bytes read
// - if called with `len == 0`, seek/synchronize to `offset` without reading, return 0 on success, non-zero for failure
typedef size_t (*gguf_reader_callback_t)(void * userdata, void * output, size_t offset, size_t len);

This should I think be the specification for the callback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


GGML_API struct gguf_context * gguf_init_empty(void);
GGML_API struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_params params);
GGML_API struct gguf_context * gguf_init_from_file(const char * fname, struct gguf_init_params params);
//GGML_API struct gguf_context * gguf_init_from_buffer(..);
GGML_API struct gguf_context * gguf_init_from_buffer(const void * data, size_t size, struct gguf_init_params params);

// when `total_size == 0` then end of file offset will be determined when the returned read size is smaller than the requested `len`
GGML_API struct gguf_context * gguf_init_from_callback(gguf_reader_callback_t callback, void * userdata, size_t max_chunk_read, size_t total_size, struct gguf_init_params params);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// when `total_size == 0` then end of file offset will be determined when the returned read size is smaller than the requested `len`
GGML_API struct gguf_context * gguf_init_from_callback(gguf_reader_callback_t callback, void * userdata, size_t max_chunk_read, size_t total_size, struct gguf_init_params params);
GGML_API struct gguf_context * gguf_init_from_callback(gguf_reader_callback_t callback, void * userdata, size_t max_chunk_read, size_t max_expected_size, struct gguf_init_params params);

The total size should not be determined from file reads. That circumvents the hardening in gguf.cpp against overflow-based vulnerabilities. The user should always explicitly provide an upper bound for what they expect the maximum file size to be, even if that is 10x higher than the actual file size it should still be good enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


GGML_API void gguf_free(struct gguf_context * ctx);

Expand Down
203 changes: 176 additions & 27 deletions ggml/src/gguf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,19 @@ struct gguf_context {
};

struct gguf_reader {
gguf_reader(FILE * file) : file(file) {
// read the remaining bytes once and update on each read
nbytes_remain = file_remain(file);
gguf_reader(
gguf_reader_callback_t callback,
void * userdata,
size_t max_chunk_read,
size_t data_offset = 0,
uint64_t nbytes_remain = 0,
bool has_nbytes_remain = false)
: callback(callback),
userdata(userdata),
max_chunk_read(max_chunk_read),
data_offset(data_offset),
nbytes_remain(nbytes_remain),
has_nbytes_remain(has_nbytes_remain) {
}

// helper for remaining bytes in a file
Expand All @@ -257,12 +267,10 @@ struct gguf_reader {
template <typename T>
bool read(T & dst) const {
const size_t size = sizeof(dst);
if (nbytes_remain < size) {
if (has_nbytes_remain && nbytes_remain < size) {
return false;
}
const size_t nread = fread(&dst, 1, size, file);
nbytes_remain -= nread;
return nread == size;
return read_raw(&dst, size) == size;
}

template <typename T>
Expand All @@ -275,14 +283,14 @@ struct gguf_reader {
if (n > SIZE_MAX / sizeof(uint64_t)) {
return false;
}
if (nbytes_remain < n * sizeof(uint64_t)) {
if (has_nbytes_remain && nbytes_remain < n * sizeof(uint64_t)) {
return false;
}
} else {
if (n > SIZE_MAX / sizeof(T)) {
return false;
}
if (nbytes_remain < n * sizeof(T)) {
if (has_nbytes_remain && nbytes_remain < n * sizeof(T)) {
return false;
}
}
Expand Down Expand Up @@ -339,29 +347,89 @@ struct gguf_reader {
GGML_LOG_ERROR("%s: string length %" PRIu64 " exceeds maximum %" PRIu64 "\n", __func__, size, (uint64_t) GGUF_MAX_STRING_LENGTH);
return false;
}
if (size > nbytes_remain) {
if (has_nbytes_remain && size > nbytes_remain) {
GGML_LOG_ERROR("%s: string length %" PRIu64 " exceeds remaining file size %" PRIu64 " bytes\n", __func__, size, nbytes_remain);
return false;
}
dst.resize(static_cast<size_t>(size));
const size_t nread = fread(dst.data(), 1, size, file);
nbytes_remain -= nread;
return nread == size;
return read_raw(dst.data(), static_cast<size_t>(size)) == size;
}

bool read(void * dst, const size_t size) const {
if (size > nbytes_remain) {
if (has_nbytes_remain && size > nbytes_remain) {
return false;
}
const size_t nread = fread(dst, 1, size, file);
nbytes_remain -= nread;
return nread == size;
return read_raw(dst, size) == size;
}

uint64_t tell() const {
return data_offset;
}

bool seek(uint64_t absolute_offset) const {
if (absolute_offset > SIZE_MAX) {
return false;
}

const uint64_t end_offset = uint64_t(data_offset) + nbytes_remain;
if (has_nbytes_remain && absolute_offset > end_offset) {
return false;
}

const size_t offset = static_cast<size_t>(absolute_offset);
if (offset != data_offset && callback(userdata, nullptr, offset, 0) != 0) {
return false;
}

data_offset = offset;
if (has_nbytes_remain) {
nbytes_remain = end_offset - absolute_offset;
}

return true;
}

private:
FILE * file;
size_t read_raw(void * dst, size_t size) const {
if (callback == nullptr) {
return 0;
}

mutable uint64_t nbytes_remain;
uint8_t * data = static_cast<uint8_t *>(dst);
size_t total_nread = 0;
bool reached_eof = false;

while (total_nread < size) {
const size_t chunk = std::min(max_chunk_read, size - total_nread);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const size_t chunk = std::min(max_chunk_read, size - total_nread);
const size_t chunk_size = std::min(max_chunk_read, size - total_nread);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

if (data_offset + total_nread < data_offset) {
break;
}
const size_t nread = callback(userdata, data + total_nread, data_offset + total_nread, chunk);
total_nread += nread;
if (nread != chunk) {
reached_eof = true;
break;
}
}

data_offset += total_nread;
if (has_nbytes_remain) {
GGML_ASSERT(total_nread <= nbytes_remain);
nbytes_remain -= total_nread;
} else if (reached_eof) {
nbytes_remain = 0;
has_nbytes_remain = true;
}

return total_nread;
}

gguf_reader_callback_t callback = nullptr;
void * userdata = nullptr;
size_t max_chunk_read = 0;
mutable size_t data_offset = 0;
mutable uint64_t nbytes_remain = 0;
mutable bool has_nbytes_remain = false;
};

struct gguf_context * gguf_init_empty(void) {
Expand Down Expand Up @@ -394,12 +462,7 @@ bool gguf_read_emplace_helper(const struct gguf_reader & gr, std::vector<struct
return true;
}

struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_params params) {
if (!file) {
return nullptr;
}

const struct gguf_reader gr(file);
static struct gguf_context * gguf_init_from_reader(const struct gguf_reader & gr, struct gguf_init_params params) {
struct gguf_context * ctx = new gguf_context;

bool ok = true;
Expand Down Expand Up @@ -700,14 +763,14 @@ struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_para
GGML_ASSERT(int64_t(ctx->info.size()) == n_tensors);

// we require the data section to be aligned, so take into account any padding
if (gguf_fseek(file, GGML_PAD(gguf_ftell(file), ctx->alignment), SEEK_SET) != 0) {
if (!gr.seek(GGML_PAD(gr.tell(), ctx->alignment))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!gr.seek(GGML_PAD(gr.tell(), ctx->alignment))) {
if (n_tensors > 0 && !gr.seek(GGML_PAD(gr.tell(), ctx->alignment))) {

The CI is failing on the edge case of a GGUF file with 0 tensors in which case the file size is not padded. I would say to just handle that edge case like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

GGML_LOG_ERROR("%s: failed to seek to beginning of data section\n", __func__);
gguf_free(ctx);
return nullptr;
}

// store the current file offset - this is where the data section starts
ctx->offset = gguf_ftell(file);
ctx->offset = gr.tell();

// compute the total size of the data section, taking into account the alignment
{
Expand Down Expand Up @@ -844,6 +907,92 @@ struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_para
return ctx;
}

struct gguf_context * gguf_init_from_callback(gguf_reader_callback_t callback, void * userdata, size_t max_chunk_read, size_t total_size, struct gguf_init_params params) {
if (callback == nullptr || max_chunk_read == 0) {
return nullptr;
}

const struct gguf_reader gr(callback, userdata, max_chunk_read, 0, total_size, total_size != 0);
return gguf_init_from_reader(gr, params);
}

struct gguf_file_reader {
FILE * file;
size_t offset;
};

static size_t gguf_file_reader_callback(void * userdata, uint8_t * output, size_t offset, size_t len) {
gguf_file_reader & reader = *static_cast<gguf_file_reader *>(userdata);

if (reader.offset != offset) {
if (gguf_fseek(reader.file, offset, SEEK_SET) != 0) {
return len == 0 ? 1 : 0;
}

reader.offset = offset;
}

if (len == 0) {
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (len == 0) {
return 0;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

const size_t nread = fread(output, 1, len, reader.file);
reader.offset += nread;
return nread;
}

struct gguf_context * gguf_init_from_file_ptr(FILE * file, struct gguf_init_params params) {
if (!file) {
return nullptr;
}

const int64_t cur = gguf_ftell(file);
if (cur < 0) {
return nullptr;
}

gguf_file_reader reader = {
/*.file = */ file,
/*.offset = */ static_cast<size_t>(cur),
};
const struct gguf_reader gr(gguf_file_reader_callback, &reader, SIZE_MAX, reader.offset, gguf_reader::file_remain(file), true);
return gguf_init_from_reader(gr, params);
}

struct gguf_buffer_reader {
const uint8_t * data;
size_t size;
};

static size_t gguf_buffer_reader_callback(void * userdata, uint8_t * output, size_t offset, size_t len) {
const gguf_buffer_reader & reader = *static_cast<gguf_buffer_reader *>(userdata);

if (offset > reader.size) {
return len == 0 ? 1 : 0;
}

if (len == 0) {
return 0;
}
Comment thread
JohannesGaessler marked this conversation as resolved.

const size_t nread = std::min(len, reader.size - offset);
memcpy(output, reader.data + offset, nread);
return nread;
}

struct gguf_context * gguf_init_from_buffer(const void * data, size_t size, struct gguf_init_params params) {
if (data == nullptr || size == 0) {
return nullptr;
}

gguf_buffer_reader reader = {
/*.data = */ static_cast<const uint8_t *>(data),
/*.size = */ size,
};
const struct gguf_reader gr(gguf_buffer_reader_callback, &reader, SIZE_MAX, 0, size, true);
return gguf_init_from_reader(gr, params);
}

struct gguf_context * gguf_init_from_file(const char * fname, struct gguf_init_params params) {
FILE * file = ggml_fopen(fname, "rb");

Expand Down
Loading