Skip to content

Charset encoding fix #8955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Ev3nt wants to merge 2 commits into
base: master
Choose a base branch
from
+35 −50
Open

Charset encoding fix #8955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f28c1a3
Charset encoding fix
Ev3nt Mar 26, 2026
dc22270
Fix integer truncation in convertLength buffer size calculation
Ev3nt Mar 26, 2026
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
43 changes: 8 additions & 35 deletions src/dsql/dsql.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,8 +594,6 @@ static RefPtr<DsqlStatement> prepareStatement(thread_db* tdbb, dsql_dbb* databas

const auto charSetId = database->dbb_attachment->att_charset;

string transformedText;

{ // scope to delete parser before the scratch pool is gone
Jrd::ContextPoolHolder scratchContext(tdbb, scratchPool);

Expand All @@ -618,41 +616,16 @@ static RefPtr<DsqlStatement> prepareStatement(thread_db* tdbb, dsql_dbb* databas
if (parser.isStmtAmbiguous())
scratch->flags |= DsqlCompilerScratch::FLAG_AMBIGUOUS_STMT;

transformedText = parser.getTransformedString();
}
const string& source = parser.getTransformedString();
string transformedText(*scratchPool);

// If the attachment charset is NONE, replace non-ASCII characters by question marks, so
// that engine internals doesn't receive non-mappeable data to UTF8. If an attachment
// charset is used, validate the string.
if (charSetId == CS_NONE)
{
for (char* p = transformedText.begin(), *end = transformedText.end(); p < end; ++p)
{
if (UCHAR(*p) > 0x7F)
*p = '?';
}
// If the attachment charset is NONE, we first try to convert data to UTF8;
// and if that fails, replace non-ASCII characters by question marks.
static_assert(CS_METADATA == CS_UTF8);
const bool isConverted = DataTypeUtil::convertToUTF8(source, transformedText, charSetId, ERRD_post);
dsqlStatement->setSqlText(FB_NEW_POOL(*statementPool) RefString(*statementPool,
isConverted ? transformedText : source));
Copy link
Copy Markdown
Member

@hvlad hvlad Mar 26, 2026

Choose a reason for hiding this comment

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

In which pool the source was allocated ?

Copy link
Copy Markdown
Contributor Author

@Ev3nt Ev3nt Mar 26, 2026

Choose a reason for hiding this comment

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

As I understand from the code, in defaultMemoryPool.

Copy link
Copy Markdown
Member

@hvlad hvlad Mar 26, 2026

Choose a reason for hiding this comment

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

I see that Parser and its transformedString allocated from scratchPool

Copy link
Copy Markdown
Contributor

@aafemt aafemt Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Line 626 explicitly allocates the resulting string in statementPool, no?

Copy link
Copy Markdown
Contributor Author

@Ev3nt Ev3nt Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

transformedString uses a default constructor that calls getDefaultMemoryPool(), no?

Copy link
Copy Markdown
Contributor

@aafemt aafemt Mar 26, 2026

Choose a reason for hiding this comment

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

RefString is not "a reference to string", it is "a reference-counted string". It is explicitly allocated in the statement pool and is given with the statement pool for copying content from transformedText or source, so it doesn't matter where Parser and transformedString is allocated, the text will end up in the statement pool.

Copy link
Copy Markdown
Member

@hvlad hvlad Mar 26, 2026

Choose a reason for hiding this comment

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

Finally something meaningful...

Copy link
Copy Markdown
Contributor

@aafemt aafemt Mar 26, 2026

Choose a reason for hiding this comment

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

We could save some time if type names in Firebird were meaningful.

Copy link
Copy Markdown
Member

@hvlad hvlad Mar 26, 2026

Choose a reason for hiding this comment

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

We could save some time if type names in Firebird were meaningful.

Not in this case, for sure

Copy link
Copy Markdown
Member

@hvlad hvlad Mar 26, 2026

Choose a reason for hiding this comment

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

BTW, I would reconsider needs to copy parser's transformedString into separate temporary string (transformedText) or, at least, allocate transformedText from short-lived scratchPool to not waste space in statementPool

}
else
{
CharSet* charSet = INTL_charset_lookup(tdbb, charSetId);

if (!charSet->wellFormed(transformedText.length(),
(const UCHAR*) transformedText.begin(), NULL))
{
ERRD_post(Arg::Gds(isc_sqlerr) << Arg::Num(-104) <<
Arg::Gds(isc_malformed_string));
}

UCharBuffer temp;

CsConvert conversor(charSet->getStruct(),
INTL_charset_lookup(tdbb, CS_METADATA)->getStruct());
conversor.convert(transformedText.length(), (const UCHAR*) transformedText.c_str(), temp);

transformedText.assign(temp.begin(), temp.getCount());
}

dsqlStatement->setSqlText(FB_NEW_POOL(*statementPool) RefString(*statementPool, transformedText));

// allocate the send and receive messages

Expand Down
38 changes: 25 additions & 13 deletions src/jrd/DataTypeUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,10 @@ ULONG DataTypeUtilBase::convertLength(ULONG len, CSetId srcCharSet, CSetId dstCh
if (dstCharSet == CS_NONE || dstCharSet == CS_BINARY)
return len;

return (len / maxBytesPerChar(srcCharSet)) * maxBytesPerChar(dstCharSet);
const ULONG srcBPC = maxBytesPerChar(srcCharSet);
const ULONG dstBPC = maxBytesPerChar(dstCharSet);

return (ROUNDUP(len, srcBPC) / srcBPC) * dstBPC;
}


Expand Down Expand Up @@ -376,28 +379,37 @@ bool DataTypeUtil::convertToUTF8(const string& src, string& dst, CSetId charset,
if (charset == CS_UTF8 || charset == CS_UNICODE_FSS)
return false;

if (charset == CS_NONE)
{
const FB_SIZE_T length = src.length();

const char* s = src.c_str();
char* p = dst.getBuffer(length);

for (const char* end = src.end(); s < end; ++p, ++s)
*p = (*s < 0 ? '?' : *s);
}
else // charset != CS_UTF8
// We throw a status_exception exception to catch it and check charset again.
// If charset is NONE, we re-throw the exception through err().
try
{
DataTypeUtil dtUtil(tdbb);
ULONG length = dtUtil.convertLength(src.length(), charset, CS_UTF8);

length = INTL_convert_bytes(tdbb,
CS_UTF8, (UCHAR*) dst.getBuffer(length), length,
charset, (const BYTE*) src.begin(), src.length(),
err);
status_exception::raise);

dst.resize(length);
}
catch (const status_exception& ex)
{
const Arg::StatusVector v(ex);

if (charset == CS_NONE)
{
const FB_SIZE_T length = src.length();

const char* s = src.c_str();
char* p = dst.getBuffer(length);

for (const char* end = src.end(); s < end; ++p, ++s)
*p = (*s < ASCII_SPACE ? '?' : *s);
}
else
err(v);
}

return true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/jrd/intl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ struct IdStorage
constexpr explicit IdStorage(USHORT id) : val(id) { }

constexpr operator USHORT() const { return val; }
bool operator==(const IdStorage& id) const { return val == id.val; }
bool operator!=(const IdStorage& id) const { return val != id.val; }
constexpr bool operator==(const IdStorage& id) const { return val == id.val; }
constexpr bool operator!=(const IdStorage& id) const { return val != id.val; }

private:
USHORT val;
Expand Down
Loading