Repository navigation
Avoid undefined behavior on non-ASCII bytes in the text date and timestamp decoders - #761
Merged
Merged
Conversation
isdigit() takes an int that must be representable as unsigned char or EOF. A plain char >= 0x80 is negative on most platforms, which is undefined behavior: the Windows debug CRT asserts and its release CRT reads before its ctype table. rb_isdigit() compares against '0'..'9' and accepts any int.
Substitute ² at every position of the invalid characters loop, and keep the date samples 10 bytes long so they reach the digit checks.
That's ² in Latin-1 and CP1252, where a locale aware isdigit() may see a digit.
The decoder specs can only catch these calls on a Windows debug CRT, which no CI job uses, and ged#758 brought isdigit() back without notice. Scanning the sources fails on every platform.
pg.c and pg_tuple.c hold UTF-8 bytes, which File.readlines rejects where the default external encoding is US-ASCII, as in the fedora:39 container without a locale.
Contributor
Author
|
Thanks @larskanis ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Another one spotted while working on #759, after #760: the text date and timestamp decoders call
isdigit()on plainchars.isdigit()only accepts values representable asunsigned char(orEOF), and a byte >= 0x80 is negative wherevercharis signed, which is undefined behavior. glibc and macOS happen to return false, but the Windows debug CRT asserts on it, and other C libraries are free to do anything. The date decoder I added in #758 has the same problem.This switches the 28 calls to
rb_isdigit(), whichruby.hprovides as an inline that compares against'0'..'9'and accepts anyint, in the same spirit as the localarray_isspace().<ctype.h>isn't needed anymore in that file.PostgreSQL never sends such bytes in a date or timestamp, but they can reach these decoders through a direct
decodecall, or a type map applying them to other data, e.g.CopyRowwith escaped bytes orFromBase64. The specs now substitute²at every position of the existing invalid characters loop, both in UTF-8 (\xC2\xB2) and as a lone\xB2byte (²in Latin-1 and CP1252, which a locale awareisdigit()might take for a digit), and add 10-byte date samples with both where digits are expected. On Linux and macOS they pass with or without this change, since those libcs tolerate negative values. To check they'd catch a regression, I built master with anisdigit()wrapper that raises outside-1..255, as the Windows debug CRT asserts: both examples fail there and pass on this branch. Since no CI job uses a debug CRT, and #758 broughtisdigit()back without anything noticing, a last spec inpg_spec.rbscansext/*.[ch]forctype.hcalls outside comments. It fails on every platform on master, listing the 28 calls, and passes here. Too much for your taste? I'm happy to drop it.It doesn't conflict with #759, which touches the same function.
🤖 Generated with Claude Code, but prompted, read, and verified by @czj, a caring human :)