Skip to content

Blob._write_to_queue() never detects an invalid commit_id — the error check is dead code #1478

Description

@K-ANOY

Blob__write_to_queue() checks the result of py_oid_to_git_oid() with if (err < 0), but that function returns size_t and signals failure by returning 0. The branch can never be taken, so an invalid commit_id is silently ignored and a Python exception is left set while execution continues.

File: src/blob.c

Function: Blob__write_to_queue (line 281), at line 319

int err;
...
if (py_oid != NULL && py_oid != Py_None)
{
    err = py_oid_to_git_oid(py_oid, &opts.attr_commit_id);
    if (err < 0)
        return Error_set(err);
}

py_oid_to_git_oid() (src/oid.c, line 88) is declared size_t and returns the oid length on success, 0 on failure:

size_t
py_oid_to_git_oid(PyObject *py_oid, git_oid *oid)
{
    /* Oid */
    if (PyObject_TypeCheck(py_oid, (PyTypeObject*)&OidType)) {
        git_oid_cpy(oid, &((Oid*)py_oid)->oid);
        return GIT_OID_HEXSZ;
    }

    /* Hex */
    return py_hex_to_git_oid(py_oid, oid);
}

The value is 40 or 0, never negative, so if (err < 0) is never true. Every other call site in the tree uses the correct convention, for example src/treebuilder.c:70:

len = py_oid_to_git_oid(py_oid, &oid);
if (len == 0)
    return NULL;

src/blob.c:319 is the only one that tests for a negative value.

commit_id is reachable from the public API: BlobIO(blob, as_path='f.txt', commit_id=...) passes it straight through to _write_to_queue. When it is not a valid Oid or hex string:

  1. py_hex_to_git_oid() sets a Python exception (TypeError, or the git error for a malformed hex string) and returns 0.
  2. The check does not fire, so opts.attr_commit_id keeps the zero value from GIT_BLOB_FILTER_OPTIONS_INIT. Filtering proceeds against a null commit id instead of failing, producing silently wrong filtered content when ATTRIBUTES_FROM_COMMIT is in play.
  3. The exception stays set across git_filter_list_load_ext(), Py_BEGIN_ALLOW_THREADS and the filter callbacks, and the function finally reaches Py_RETURN_NONE. Returning a result with a live exception makes _Py_CheckFunctionResult() raise SystemError: ... returned a result with an exception set.

Because _BlobIO runs _write_to_queue on a worker thread, that SystemError surfaces in an unrelated thread with a traceback that points nowhere near the bad argument.

Suggested fix

size_t len = py_oid_to_git_oid(py_oid, &opts.attr_commit_id);
if (len == 0) {
    git_blob_free(blob);
    return NULL;   /* py_oid_to_git_oid already set the exception */
}

Error_set(err) is not appropriate here — the failure is reported through the Python exception that py_hex_to_git_oid() already set, not through a libgit2 error code. Note that the current early return also skips git_blob_free(blob), unlike the git_filter_list_load_ext() failure path directly below it, so the replacement needs to free the blob.

Related: call sites that discard the result entirely

Several other callers ignore the return value, which has the same effect of continuing with an unset oid and a dangling exception:

  • src/reference.c:168 and src/reference.c:170
  • src/refdb_backend.c:601 and src/refdb_backend.c:675
  • src/odb_backend.c:127, :207, :231

These are worth auditing alongside the fix above.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions