From 0f172f9d5e1ee3d366bcb59a243dd9c03d80404d Mon Sep 17 00:00:00 2001 From: Michael Wegener Date: Mon, 20 Jul 2026 11:46:54 +0200 Subject: [PATCH] fix(blob): #516 - fix 32-bit truncation in BLOB ID round-trip + legacy compat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two critical bugs found by code review before v13.0.1 release: ## Bug 1: _php_fbird_string_to_quad truncates 32-bit low part to 16 bits fbird_blobs.c:225: cast was (ISC_USHORT)low_part — truncates 32-bit ISC_ULONG to 16 bits. The #516 fix changed the variable type and sscanf format but forgot to update the cast on the assignment line. Fix: (ISC_ULONG)low_part (matches gds_quad_low type in types_pub.h:195) ## Bug 2: BlobId::fromString() drops lower 16 bits of hex-format input src/Firebird/BlobId.php:140: substr($hex, 8, 4) reads only 4 hex digits (16 bits) instead of 8 (32 bits). The #516 fix changed the output format (%08X) but not the input parsing. Test blessed the buggy output. Fix: substr($hex, 8, 8) + update test expectation. ## Issue 3: Procedural binding rejects 13-char BLOB IDs (asymmetric) fbird_query_bind.c:929/1043, fbird_batch.c:560/741 used exact Z_STRLEN_P != BLOB_ID_LEN (17) check, rejecting legacy 13-char IDs from v13.0.0. Users upgrading could not bind stored 13-char IDs — silently treated as blob content (data corruption). Fix: Accept both BLOB_ID_LEN (17) and BLOB_ID_LEN_LEGACY (13). _php_fbird_string_to_quad() already parses both via %x:%x. ## Cosmetic fixes - fbird_batch.c:554: removed duplicate comment block + stale example - BlobId.php: updated stale docblocks (13→17 char format) - php_fbird_includes.h: added BLOB_ID_LEN_LEGACY constant ## ISC_QUAD structure (verified across FB3/FB4/FB5) gds_quad_high: ISC_LONG (32-bit signed) gds_quad_low: ISC_ULONG (32-bit unsigned) → 17-char format (8+1+8 hex) is correct for ALL Firebird versions ## Verified on CI-exact environment - php:8.4-cli-bookworm + official FB4 tarball - firebirdsql/firebird:4 + firebirdsql/firebird:5 servers - tests/blobid_001.phpt PASS (FB4 + FB5) - tests/003.phpt PASS (FB4 + FB5) - tests/issue119.phpt PASS - tests/issue307_trans_start_nullable_options.phpt PASS - tests/transaction_default_slot.phpt PASS Found by: v13.0.1 release readiness code review --- fbird_batch.c | 12 ++++-------- fbird_blobs.c | 2 +- fbird_query_bind.c | 4 ++-- php_fbird_includes.h | 4 +++- src/Firebird/BlobId.php | 12 +++++------- tests/blobid_001.phpt | 2 +- 6 files changed, 16 insertions(+), 20 deletions(-) diff --git a/fbird_batch.c b/fbird_batch.c index 96eedb8f..ff7e220d 100644 --- a/fbird_batch.c +++ b/fbird_batch.c @@ -554,14 +554,10 @@ int fbird_batch_add_impl(fbird_batch *fb_batch, zval *args, int argc) /* BLOB ID as hex string "HHHHHHHH:LLLLLLLL" (17 characters) * Format: 8 hex digits (high 32-bit), colon, 8 hex digits (low 32-bit) * jane: #516 - was 13 chars (4 hex low), now 17 chars (8 hex low, full 32-bit) - /* BLOB ID as hex string "HHHHHHHH:LLLLLLLL" (17 characters) - * Format: 8 hex digits (high 32-bit), colon, 8 hex digits (low 32-bit) - * jane: #516 - was 13 chars (4 hex low), now 17 chars (8 hex low, full 32-bit) - * Example: "74292B00:7FFC" - */ + * Example: "74292B00:00007FFC" */ convert_to_string(b_var); - if (Z_STRLEN_P(b_var) == BLOB_ID_LEN && + if (((Z_STRLEN_P(b_var) == BLOB_ID_LEN || Z_STRLEN_P(b_var) == BLOB_ID_LEN_LEGACY)) && _php_fbird_string_to_quad(Z_STRVAL_P(b_var), (ISC_QUAD *)data_ptr)) { /* Valid BLOB ID parsed and written to message buffer */ break; @@ -742,8 +738,8 @@ PHP_FUNCTION(fbird_batch_register_blob) } /* Validate and convert BLOB ID string to ISC_QUAD */ - if (blob_id_len != BLOB_ID_LEN || !_php_fbird_string_to_quad(blob_id_str, &existing_blob)) { - _php_fbird_module_error("Invalid BLOB ID format (expected %d character hex string)", BLOB_ID_LEN); + if ((blob_id_len != BLOB_ID_LEN && blob_id_len != BLOB_ID_LEN_LEGACY) || !_php_fbird_string_to_quad(blob_id_str, &existing_blob)) { + _php_fbird_module_error("Invalid BLOB ID format (expected %d or %d character hex string)", BLOB_ID_LEN, BLOB_ID_LEN_LEGACY); RETURN_FALSE; } diff --git a/fbird_blobs.c b/fbird_blobs.c index b9d8fd05..03e6e088 100755 --- a/fbird_blobs.c +++ b/fbird_blobs.c @@ -222,7 +222,7 @@ int _php_fbird_string_to_quad(char const *id, ISC_QUAD *qd) if (sscanf(id, "%x:%x", &high_part, &low_part) == 2) { qd->gds_quad_high = (ISC_LONG)high_part; - qd->gds_quad_low = (ISC_USHORT)low_part; + qd->gds_quad_low = (ISC_ULONG)low_part; return 1; } diff --git a/fbird_query_bind.c b/fbird_query_bind.c index 005c4786..034bc611 100755 --- a/fbird_query_bind.c +++ b/fbird_query_bind.c @@ -926,7 +926,7 @@ int _php_fbird_bind(fbird_query *fb_query, zval *b_vars) convert_to_string(b_var); - if (Z_STRLEN_P(b_var) != BLOB_ID_LEN || + if ((Z_STRLEN_P(b_var) != BLOB_ID_LEN && Z_STRLEN_P(b_var) != BLOB_ID_LEN_LEGACY) || !_php_fbird_string_to_quad(Z_STRVAL_P(b_var), &buf[i].val.qval)) { /* OO API only: create a blob, write the string into it, then bind by blob id (ISC_QUAD). */ @@ -1040,7 +1040,7 @@ int _php_fbird_bind(fbird_query *fb_query, zval *b_vars) if (Z_TYPE_P(b_var) != IS_ARRAY) { convert_to_string(b_var); - if (Z_STRLEN_P(b_var) != BLOB_ID_LEN || + if ((Z_STRLEN_P(b_var) != BLOB_ID_LEN && Z_STRLEN_P(b_var) != BLOB_ID_LEN_LEGACY) || !_php_fbird_string_to_quad(Z_STRVAL_P(b_var), &buf[i].val.qval)) { _php_fbird_module_error("Parameter %d: invalid array ID",i+1); diff --git a/php_fbird_includes.h b/php_fbird_includes.h index b6ce7e4d..1a1b3d31 100755 --- a/php_fbird_includes.h +++ b/php_fbird_includes.h @@ -319,8 +319,10 @@ ZEND_TSRMLS_CACHE_EXTERN() * gds_quad_low is ISC_ULONG (32-bit unsigned) per firebird/impl/types_pub.h:194. * BLOB_ID_MASK changed from %x:%hx (16-bit) to %x:%x (full 32-bit). * Old 13-char IDs still parse correctly via _php_fbird_string_to_quad (uses %x:%x). + * BLOB_ID_LEN_LEGACY is for backwards-compat length checks in binding paths. */ -#define BLOB_ID_LEN 17 +#define BLOB_ID_LEN 17 +#define BLOB_ID_LEN_LEGACY 13 #define BLOB_ID_MASK "%x:%x" #define BLOB_INPUT 1 diff --git a/src/Firebird/BlobId.php b/src/Firebird/BlobId.php index 808f1fdb..f8c5ead4 100755 --- a/src/Firebird/BlobId.php +++ b/src/Firebird/BlobId.php @@ -106,7 +106,7 @@ private function __construct(string $id, int $high, int $low) * Create a BlobId from a string. * * Supports two formats: - * - Colon format: "HHHHHHHH:LLLL" (13 chars, standard from php-firebird extension) + * - Colon format: "HHHHHHHH:LLLLLLLL" (17 chars, standard from php-firebird extension) * - Hex format: "0xHHHHHHHHHHHHHHHH" (18 chars, legacy) * * @param string $id BLOB ID string @@ -119,7 +119,7 @@ public static function fromString(string $id): self if (!self::isValidFormat($id)) { throw new InvalidArgumentException(sprintf( - 'Invalid BLOB ID format: "%s". Expected formats: "HHHHHHHH:LLLL" or "0xHHHHHHHHHHHHHHHH".', + 'Invalid BLOB ID format: "%s". Expected formats: "HHHHHHHH:LLLLLLLL" or "0xHHHHHHHHHHHHHHHH".', strlen($id) > 30 ? substr($id, 0, 30) . '...' : $id )); } @@ -132,12 +132,10 @@ public static function fromString(string $id): self $low = hexdec($lowHex); $normalized = sprintf('%08X:%08X', $high, $low); } else { - // Hex format: "0xHHHHHHHHHHHHHHHH" - // Colon format uses high 32 bits + middle 16 bits (not last 16) + // Hex format: "0xHHHHHHHHLLLLLLLL" (16 hex digits = 64 bits) $hex = substr($id, 2); // Remove "0x" prefix - $high = hexdec(substr($hex, 0, 8)); - // For colon format, use middle 16 bits (positions 8-11, not 12-15) - $low = hexdec(substr($hex, 8, 4)); + $high = hexdec(substr($hex, 0, 8)); // first 8 hex = high 32 bits + $low = hexdec(substr($hex, 8, 8)); // next 8 hex = low 32 bits // Normalize to colon format (standard) $normalized = sprintf('%08X:%08X', $high, $low); } diff --git a/tests/blobid_001.phpt b/tests/blobid_001.phpt index 7e22dd0a..93392545 100755 --- a/tests/blobid_001.phpt +++ b/tests/blobid_001.phpt @@ -163,7 +163,7 @@ bool(false) Test 10: Case insensitivity and format conversion bool(true) string(17) "abcdef01:00002345" -string(17) "abcdef01:00002345" +string(17) "abcdef01:23456789" Test 11: Integration with fbird_blob functions bool(true)