diff --git a/bindings/python/tests/test_utf8_paths.py b/bindings/python/tests/test_utf8_paths.py new file mode 100644 index 00000000..ea09bc44 --- /dev/null +++ b/bindings/python/tests/test_utf8_paths.py @@ -0,0 +1,87 @@ +"""Non-ASCII (UTF-8) path handling through the C ABI. + +Regression coverage for Handy issue #1585: on Windows, model loading +and backend artifact-dir validation failed for non-ASCII paths because +the library's narrow ``::stat()`` / ``ifstream`` calls read the UTF-8 +path bytes in the process ANSI code page. src/transcribe-path.h now +converts UTF-8 to wide paths on Windows. + +The C++ counterpart (tests/utf8_path_unit.cpp) pins the same contract, +but ctest only runs on Linux/macOS in CI — this file is what executes +on a real Windows runner (the wheel lanes run the pytest suite via +scripts/ci/wheel_smoke.py). + +Model-free tests always run; the full-load test skips without a model +(``TRANSCRIBE_SMOKE_MODEL``, set by the wheel lanes). +""" + +from __future__ import annotations + +import os +import shutil +from pathlib import Path + +import pytest + +import transcribe_cpp as t +from transcribe_cpp import errors + +# Same characters as the C++ counterpart's directory name. +NON_ASCII_DIR = "transcribe-utf8-café-日本語" + + +def _nonascii_dir(tmp_path: Path) -> Path: + d = tmp_path / NON_ASCII_DIR + try: + d.mkdir() + except OSError as e: # filesystem that cannot represent the name + pytest.skip(f"cannot create non-ASCII directory: {e}") + return d + + +def test_missing_file_in_nonascii_dir_raises_file_not_found(tmp_path): + # The not-found classification must survive the wide-path port: a + # genuinely absent file is FILE_NOT_FOUND, not some generic error. + d = _nonascii_dir(tmp_path) + with pytest.raises(t.ModelFileNotFound) as ei: + t.Model(d / "definitely-missing.gguf") + assert ei.value.status == errors.ERR_FILE_NOT_FOUND + + +def test_junk_file_in_nonascii_dir_is_found_then_rejected(tmp_path): + # The #1585 failure shape: the file EXISTS at the non-ASCII path, + # but pre-fix Windows raised ModelFileNotFound (the ANSI-code-page + # stat could not see it). ModelLoadError is a sibling class, so + # this cannot pass via FILE_NOT_FOUND. + d = _nonascii_dir(tmp_path) + junk = d / "junk.gguf" + junk.write_bytes(b"this is not a gguf file" * 64) + with pytest.raises(t.ModelLoadError): + t.Model(junk) + + +def test_smoke_model_loads_from_nonascii_dir(model_path, tmp_path): + # Full successful load through the non-ASCII path: existence + # pre-check, magic sniff, gguf parse, and the tensor-data streaming + # reopen all consume the path. Constructing the Model is the + # assertion. + d = _nonascii_dir(tmp_path) + local = d / model_path.name + shutil.copyfile(model_path, local) + with t.Model(local): + pass + + +def test_init_backends_nonascii_dirs(tmp_path): + # The existing non-ASCII dir must not be misread as absent; + # FILE_NOT_FOUND specifically is the #1585 encoding bug. We do not + # assert OK: a module-less dir may legitimately report ERR_BACKEND + # depending on build posture. + lib = t._lib + d = _nonascii_dir(tmp_path) + st = lib.transcribe_init_backends(os.fspath(d).encode("utf-8")) + assert st != errors.ERR_FILE_NOT_FOUND + + missing = d / "missing-subdir" + st = lib.transcribe_init_backends(os.fspath(missing).encode("utf-8")) + assert st == errors.ERR_FILE_NOT_FOUND diff --git a/include/transcribe.h b/include/transcribe.h index 15bf1d67..27cc3f03 100644 --- a/include/transcribe.h +++ b/include/transcribe.h @@ -719,6 +719,9 @@ typedef enum { * is automatic under the supported usage (call once, before the first * model load). * + * artifact_dir is UTF-8, like every path crossing this API; on Windows + * it is converted to a wide path internally. + * * Returns: * TRANSCRIBE_ERR_INVALID_ARG artifact_dir is NULL or empty. * TRANSCRIBE_ERR_FILE_NOT_FOUND artifact_dir is not an existing directory. @@ -1335,6 +1338,10 @@ TRANSCRIBE_API const char * transcribe_model_meta_val_str(const struct transcrib /* * Load a GGUF model from disk. * + * path is UTF-8, like every path crossing this API; on Windows it is + * converted to a wide path internally, so non-ASCII paths load + * correctly regardless of the process ANSI code page. + * * params may be NULL for all library defaults. To customize, initialize * a struct with transcribe_model_load_params_init() and set fields. A * struct with struct_size == 0 (including `{0}`) is rejected with diff --git a/src/transcribe-bin-loader.cpp b/src/transcribe-bin-loader.cpp index e2a40b42..bd95d43e 100644 --- a/src/transcribe-bin-loader.cpp +++ b/src/transcribe-bin-loader.cpp @@ -5,10 +5,8 @@ #include "ggml-backend.h" #include "ggml.h" #include "transcribe-log.h" +#include "transcribe-path.h" -#include - -#include #include #include #include @@ -20,21 +18,6 @@ namespace { constexpr const char * kTag = "whisper-bin"; -// Same path-existence check transcribe-loader uses for GGUFs: ENOENT / -// ENOTDIR are conclusive "file is not at this path"; anything else -// means we couldn't reach the path and the open below should surface -// the underlying error. -bool path_is_present(const char * path) { - struct stat st{}; - if (::stat(path, &st) == 0) { - return true; - } - if (errno == ENOENT || errno == ENOTDIR) { - return false; - } - return true; -} - // Read sizeof(T) bytes into `out`. Returns false on short read or // stream failure. template bool read_pod(std::ifstream & fin, T & out) { @@ -158,7 +141,7 @@ transcribe_status parse_whisper_bin(const char * path, WhisperBinModel & out) { } out.path = path; - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to open %s", kTag, path); return TRANSCRIBE_ERR_GGUF; @@ -395,7 +378,7 @@ transcribe_status parse_whisper_bin(const char * path, WhisperBinModel & out) { transcribe_status stream_tensor_data_from_bin(const std::string & path, const std::vector & slots, const char * error_tag) { - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to reopen %s for tensor data", error_tag, path.c_str()); return TRANSCRIBE_ERR_GGUF; diff --git a/src/transcribe-load-common.cpp b/src/transcribe-load-common.cpp index fbaf1be5..f1205905 100644 --- a/src/transcribe-load-common.cpp +++ b/src/transcribe-load-common.cpp @@ -11,6 +11,7 @@ #include "gguf.h" #include "transcribe-backend.h" #include "transcribe-log.h" +#include "transcribe-path.h" #include #include @@ -382,7 +383,7 @@ transcribe_status stream_tensor_data(const std::string & path, // time: ifstream::seekg takes a streamoff (signed 64-bit on every // platform we target), so multi-GB tensor offsets work without // #ifdef'ing fseeko vs _fseeki64. - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(path_from_utf8(path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to reopen %s for tensor data", error_tag, path.c_str()); return TRANSCRIBE_ERR_GGUF; @@ -574,7 +575,7 @@ ReadF32Result read_f32_tensor_checked(gguf_context * gguf_ctx, const size_t data_off = gguf_get_data_offset(gguf_ctx); const size_t t_off = gguf_get_tensor_offset(gguf_ctx, idx); - std::ifstream fin(gguf_path, std::ios::binary); + std::ifstream fin(path_from_utf8(gguf_path), std::ios::binary); if (!fin) { log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "%s: failed to open %s for tensor \"%s\"", error_tag, gguf_path.c_str(), tensor_name); diff --git a/src/transcribe-loader.cpp b/src/transcribe-loader.cpp index 6973783f..94603687 100644 --- a/src/transcribe-loader.cpp +++ b/src/transcribe-loader.cpp @@ -9,10 +9,7 @@ #include "gguf.h" #include "transcribe-meta.h" - -#include - -#include +#include "transcribe-path.h" namespace transcribe { @@ -29,44 +26,6 @@ gguf_context * Loader::release_gguf() { return out; } -namespace { - -// Narrow stat-based pre-check whose ONLY job is to distinguish -// "the file is not at this path" from every other reason -// gguf_init_from_file might return nullptr. -// -// We deliberately do not check S_ISREG here. If stat() succeeds and the -// path points at a directory (or a fifo, or a socket), the file IS at -// that path — it's just not a usable GGUF. fopen() inside ggml will -// return EISDIR / etc. and we will surface that as TRANSCRIBE_ERR_GGUF, -// which is strictly more accurate than collapsing it into FILE_NOT_FOUND. -// -// We also deliberately only short-circuit on ENOENT / ENOTDIR. Other -// stat() failures (EACCES, ENAMETOOLONG, ELOOP, EIO, ...) do NOT -// conclusively prove the file is missing — they prove we can't reach -// it. Returning FILE_NOT_FOUND for those would be wrong. We let -// gguf_init_from_file try and surface whatever it returns as ERR_GGUF. -// -// Returns true if the path either exists OR if we cannot prove it -// doesn't (in which case the caller should hand off to -// gguf_init_from_file unchanged). -bool path_is_present(const char * path) { - struct stat st{}; - if (::stat(path, &st) == 0) { - return true; - } - // ENOENT: the named file does not exist. - // ENOTDIR: a non-final component of the path is not a directory, - // which is the same observable failure mode as "the file - // is not at this path." - if (errno == ENOENT || errno == ENOTDIR) { - return false; - } - return true; -} - -} // namespace - transcribe_status Loader::open(const char * path) { if (path == nullptr) { return TRANSCRIBE_ERR_INVALID_ARG; @@ -74,6 +33,9 @@ transcribe_status Loader::open(const char * path) { path_ = path; + // Distinguishes "the file is not at this path" (-> FILE_NOT_FOUND) + // from every other reason gguf_init_from_file might return nullptr + // (-> ERR_GGUF). Exact semantics in transcribe-path.h. if (!path_is_present(path)) { return TRANSCRIBE_ERR_FILE_NOT_FOUND; } diff --git a/src/transcribe-path.h b/src/transcribe-path.h new file mode 100644 index 00000000..8faec1b3 --- /dev/null +++ b/src/transcribe-path.h @@ -0,0 +1,57 @@ +// transcribe-path.h - UTF-8 path handling for user-facing file access. +// +// Contract: every path crossing the public C ABI is UTF-8 (see +// include/transcribe.h). On POSIX the narrow file APIs consume UTF-8 +// bytes directly. On Windows they interpret narrow strings in the +// process ANSI code page, so a non-ASCII path fails with ENOENT even +// though the file exists (Handy issue #1585). Every stat/open a +// user-supplied path can reach must therefore go through these +// helpers, which convert UTF-8 to the native wide encoding on Windows. +// +// Deliberately not converted: dev-only I/O (the transcribe-debug.cpp +// dump writers, the TRANSCRIBE_ENABLE_VALIDATION_HOOKS sidecar +// readers) — those paths never cross the public ABI. + +#pragma once + +#include +#include +#include + +namespace transcribe { + +// Build a std::filesystem::path from UTF-8 bytes. On Windows this +// converts to the native wide encoding; elsewhere the narrow encoding +// IS UTF-8 and this is a passthrough. (fs::u8path is the C++17 +// spelling; it is deprecated-but-present under C++20.) +inline std::filesystem::path path_from_utf8(const char * utf8) { +#if defined(_WIN32) + return std::filesystem::u8path(utf8); +#else + return std::filesystem::path(utf8); +#endif +} + +inline std::filesystem::path path_from_utf8(const std::string & utf8) { + return path_from_utf8(utf8.c_str()); +} + +// Existence pre-check whose ONLY job is to distinguish "the file is +// not at this path" from every other reason a subsequent open might +// fail. +// +// Deliberately no regular-file check: if the path names a directory +// (or a fifo, or a socket), the file IS at that path — the caller's +// open will surface a more accurate error than FILE_NOT_FOUND. +// +// Deliberately only the not-found outcome (ENOENT / ENOTDIR and their +// Windows equivalents, which fs::status maps to file_type::not_found) +// reports absence. Other failures (EACCES, ELOOP, EIO, ...) don't +// prove the file is missing — only that we can't reach it — so they +// return true and the caller's open reports the real error. +inline bool path_is_present(const char * utf8_path) { + std::error_code ec; + return std::filesystem::status(path_from_utf8(utf8_path), ec).type() != std::filesystem::file_type::not_found; +} + +} // namespace transcribe diff --git a/src/transcribe.cpp b/src/transcribe.cpp index 9b203c03..6e254815 100644 --- a/src/transcribe.cpp +++ b/src/transcribe.cpp @@ -24,6 +24,7 @@ #include "transcribe-loader.h" #include "transcribe-log.h" #include "transcribe-model.h" +#include "transcribe-path.h" #include "transcribe-session.h" #include "transcribe-tokenizer.h" #include "transcribe/whisper.h" @@ -40,17 +41,8 @@ # include #endif -#include - -// MSVC's has the _S_IFDIR mode bits but not the POSIX -// S_ISDIR() macro. -#if defined(_WIN32) && !defined(S_ISDIR) -# define S_ISDIR(m) (((m) & _S_IFMT) == _S_IFDIR) -#endif - #include #include -#include #include #include #include @@ -864,8 +856,8 @@ static transcribe_status transcribe_init_backends_impl(const char * artifact_dir return TRANSCRIBE_ERR_INVALID_ARG; } { - struct stat st{}; - if (::stat(artifact_dir, &st) != 0 || !S_ISDIR(st.st_mode)) { + std::error_code ec; + if (!std::filesystem::is_directory(transcribe::path_from_utf8(artifact_dir), ec)) { transcribe::log_msg(TRANSCRIBE_LOG_LEVEL_ERROR, "transcribe_init_backends: %s is not an existing directory", artifact_dir); return TRANSCRIBE_ERR_FILE_NOT_FOUND; @@ -880,8 +872,8 @@ static transcribe_status transcribe_init_backends_impl(const char * artifact_dir std::lock_guard lock(s_mutex); std::error_code ec; - std::filesystem::path canonical = std::filesystem::weakly_canonical(artifact_dir, ec); - const std::string key = ec ? std::string(artifact_dir) : canonical.string(); + std::filesystem::path canonical = std::filesystem::weakly_canonical(transcribe::path_from_utf8(artifact_dir), ec); + const std::string key = ec ? std::string(artifact_dir) : canonical.u8string(); if (s_loaded_dirs.find(key) == s_loaded_dirs.end()) { ggml_backend_load_all_from_path(artifact_dir); s_loaded_dirs.insert(key); @@ -1419,22 +1411,15 @@ static transcribe_status transcribe_model_load_file_impl(const char * // which HF/users mislabel): GGUF magic 0x46554747 is the canonical path, // ggml magic 0x67676d6c is a legacy whisper.cpp .bin. Public contract: // missing path -> ERR_FILE_NOT_FOUND; every other failure -> ERR_GGUF. - { - struct stat st{}; - if (::stat(path, &st) != 0) { - if (errno == ENOENT || errno == ENOTDIR) { - return TRANSCRIBE_ERR_FILE_NOT_FOUND; - } - // Other stat() failures (EACCES, etc.) are not "not found"; - // the open below will surface them as ERR_GGUF. - } + if (!transcribe::path_is_present(path)) { + return TRANSCRIBE_ERR_FILE_NOT_FOUND; } uint32_t magic = 0; { - std::ifstream fin(path, std::ios::binary); + std::ifstream fin(transcribe::path_from_utf8(path), std::ios::binary); if (!fin) { - // stat() said the path is reachable but ifstream couldn't - // open it — permissions / type issue. Treat as a generic + // The existence pre-check said the path is reachable but + // the open failed — permissions / type issue. Treat as a generic // load failure rather than FILE_NOT_FOUND. return TRANSCRIBE_ERR_GGUF; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ce23f4ef..b35bb291 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -392,6 +392,37 @@ if(TRANSCRIBE_UV) SKIP_RETURN_CODE 77) endif() +# ----------------------------------------------------------------------------- +# Non-ASCII (UTF-8) path handling through the public C ABI +# ----------------------------------------------------------------------------- +# +# Regression test for Handy issue #1585: on Windows, non-ASCII paths +# failed model load and backend-dir validation because narrow +# stat()/ifstream calls read the UTF-8 bytes in the ANSI code page. +# Copies two fixtures into a non-ASCII-named temp directory and drives +# transcribe_model_load_file / transcribe_init_backends through it. On +# POSIX this pins the UTF-8 contract; on Windows it exercises the +# wide-path conversion in src/transcribe-path.h. Public-API only. +# Fixture-dependent, so gated on uv like loader_smoke. + +if(TRANSCRIBE_UV) + add_executable(transcribe_utf8_path_unit + utf8_path_unit.cpp) + + target_link_libraries(transcribe_utf8_path_unit PRIVATE transcribe) + + target_compile_definitions(transcribe_utf8_path_unit PRIVATE + "TRANSCRIBE_TEST_FIXTURES_DIR=\"${CMAKE_CURRENT_SOURCE_DIR}/fixtures\"") + + transcribe_apply_warnings(transcribe_utf8_path_unit) + + add_dependencies(transcribe_utf8_path_unit fixtures) + + add_test(NAME transcribe_utf8_path_unit COMMAND transcribe_utf8_path_unit) + set_tests_properties(transcribe_utf8_path_unit PROPERTIES + SKIP_RETURN_CODE 77) +endif() + # ----------------------------------------------------------------------------- # Tokenizer ingest smoke (success path through the public ABI) # ----------------------------------------------------------------------------- diff --git a/tests/utf8_path_unit.cpp b/tests/utf8_path_unit.cpp new file mode 100644 index 00000000..f20461a0 --- /dev/null +++ b/tests/utf8_path_unit.cpp @@ -0,0 +1,139 @@ +// utf8_path_unit.cpp - non-ASCII (UTF-8) path handling through the +// public C ABI. +// +// Regression test for Handy issue #1585: on Windows, model loading and +// backend artifact-dir validation failed for any non-ASCII path, +// because the library's ::stat() / ifstream(const std::string &) calls +// interpreted the UTF-8 path bytes in the process ANSI code page. The +// contract (include/transcribe.h) is that every path crossing the C +// ABI is UTF-8; src/transcribe-path.h now routes all file access +// through an explicit UTF-8 -> wide conversion on Windows. +// +// The test copies two build-time fixtures into a directory whose name +// contains non-ASCII characters and asserts, via the public API only: +// +// 1. A complete minimal model at the non-ASCII path loads with +// TRANSCRIBE_OK — covering the existence pre-check, the +// magic-bytes sniff, gguf_init_from_file, and the tensor-data +// streaming reopen. +// 2. arch_unknown.gguf at the non-ASCII path returns +// ERR_UNSUPPORTED_ARCH, not FILE_NOT_FOUND: the file was found +// and parsed; only the architecture is unknown. +// 3. A missing file inside the non-ASCII directory still returns +// FILE_NOT_FOUND — the not-found distinction survives the +// std::filesystem port. +// 4. transcribe_init_backends accepts the existing non-ASCII +// directory (anything but FILE_NOT_FOUND) and still rejects a +// missing non-ASCII directory with FILE_NOT_FOUND. +// +// On POSIX, narrow paths are UTF-8 natively, so cases 1-4 pin the +// contract; on Windows they exercise the actual conversion. +// +// The directory name is spelled in escaped UTF-8 bytes, not literal +// characters, so the test is immune to source-charset differences +// across compilers ("café-日本語"). + +#include "transcribe.h" + +#include +#include +#include +#include +#include + +#ifndef TRANSCRIBE_TEST_FIXTURES_DIR +# error "TRANSCRIBE_TEST_FIXTURES_DIR must be defined by the build system" +#endif + +namespace fs = std::filesystem; + +namespace { + +// "transcribe-utf8-café-日本語" in escaped UTF-8 bytes. +constexpr const char * kDirName = "transcribe-utf8-caf\xC3\xA9-\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"; + +int g_failures = 0; + +#define CHECK(cond) \ + do { \ + if (!(cond)) { \ + std::fprintf(stderr, "FAIL %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + ++g_failures; \ + } \ + } while (0) + +void check_load(const std::string & utf8_path, transcribe_status expected, const char * label) { + transcribe_model_load_params mp; + transcribe_model_load_params_init(&mp); + struct transcribe_model * m = nullptr; + + const transcribe_status st = transcribe_model_load_file(utf8_path.c_str(), &mp, &m); + + if (st != expected) { + std::fprintf(stderr, "FAIL %s: expected %s, got %s\n", label, transcribe_status_string(expected), + transcribe_status_string(st)); + ++g_failures; + } + transcribe_model_free(m); +} + +} // namespace + +int main() { + const fs::path fixtures = fs::u8path(TRANSCRIBE_TEST_FIXTURES_DIR); + const fs::path src_ok = fixtures / "tokenizer_minimal.gguf"; + const fs::path src_bad = fixtures / "arch_unknown.gguf"; + + // Backstop, same rationale as loader_smoke: the build dependency on + // the `fixtures` target guarantees these exist; skip cleanly (77) + // if someone deleted them between build and test. + std::error_code ec; + if (!fs::exists(src_ok, ec) || !fs::exists(src_bad, ec)) { + std::fprintf(stderr, + "utf8_path_unit: fixtures missing; regenerate with:\n" + " cmake --build build --target fixtures\n"); + return 77; + } + + const fs::path dir = fs::temp_directory_path() / fs::u8path(kDirName); + fs::remove_all(dir, ec); // leftovers from a previous crashed run + fs::create_directories(dir); + fs::copy_file(src_ok, dir / "tokenizer_minimal.gguf", fs::copy_options::overwrite_existing); + fs::copy_file(src_bad, dir / "arch_unknown.gguf", fs::copy_options::overwrite_existing); + + // C-API strings must be UTF-8 regardless of platform; u8string() + // guarantees that on Windows where string() would be ANSI. + const std::string dir_u8 = dir.u8string(); + + // 1. Full successful load through the non-ASCII path. This is the + // exact failure mode from Handy #1585 (FILE_NOT_FOUND despite the + // file existing). + check_load(dir_u8 + "/tokenizer_minimal.gguf", TRANSCRIBE_OK, "minimal-model"); + + // 2. The file must be FOUND and parsed; only its arch is unknown. + // A path-encoding regression turns this into FILE_NOT_FOUND. + check_load(dir_u8 + "/arch_unknown.gguf", TRANSCRIBE_ERR_UNSUPPORTED_ARCH, "unknown-arch"); + + // 3. Genuinely missing file inside the non-ASCII dir: the not-found + // classification must survive. + check_load(dir_u8 + "/__missing__.gguf", TRANSCRIBE_ERR_FILE_NOT_FOUND, "missing-file"); + + // 4. Backend artifact-dir validation. The directory exists, so + // FILE_NOT_FOUND specifically is a path-encoding bug ("...is not + // an existing directory" from Handy #1585). We do not assert OK: + // a dynamic-backend build pointed at a module-less directory + // legitimately returns ERR_BACKEND. + const transcribe_status init_st = transcribe_init_backends(dir_u8.c_str()); + CHECK(init_st != TRANSCRIBE_ERR_FILE_NOT_FOUND); + + CHECK(transcribe_init_backends((dir_u8 + "/__missing_dir__").c_str()) == TRANSCRIBE_ERR_FILE_NOT_FOUND); + + fs::remove_all(dir, ec); + + if (g_failures > 0) { + std::fprintf(stderr, "utf8_path_unit: %d failures\n", g_failures); + return EXIT_FAILURE; + } + std::fprintf(stdout, "utf8_path_unit: ok\n"); + return EXIT_SUCCESS; +}