Skip to content

Fix bitNot for String to return a zero-terminated string#80791

Merged
azat merged 1 commit intoClickHouse:masterfrom
azat:bitNot-String-NUL-terminated
May 26, 2025
Merged

Fix bitNot for String to return a zero-terminated string#80791
azat merged 1 commit intoClickHouse:masterfrom
azat:bitNot-String-NUL-terminated

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented May 25, 2025

Changelog category (leave one):

  • Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix bitNot() for String to return a zero-terminated string

Previuosly, due to bitNot ignoring row boundaries, it inverts the NUL byte as well, but some code relies on the fact that String are NUL terminated, i.e. the following will fail in debug build:

SELECT hex(any(bitNot('foo')))

With chassert() from StringValueCompatibility::getDataAtWithTerminatingZero():

Logical error: 'res.data[res.size] == '\0'

Restore NUL byte after the inversion.

Fixes: #80774

Note, I used the Backward Incompatible Change to underline this fix in changelogs, since who knows what could be broken in case of non-zero terminated strings

Previuosly, due to bitNot() ignores row boundaries, it inverts the NUL
byte as well, but some code relies on the fact that String are NUL
terminated, i.e. the following will fail in debug build:

    SELECT hex(any(bitNot('foo')))

With chassert() from StringValueCompatibility::getDataAtWithTerminatingZero():

    Logical error: 'res.data[res.size] == '\0'

Restore NUL byte after the inversion.
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented May 25, 2025

Workflow [PR], commit [1874aa6]

@clickhouse-gh clickhouse-gh bot added the pr-backward-incompatible Pull request with backwards incompatible changes label May 25, 2025
@Algunenano Algunenano self-assigned this May 26, 2025
Copy link
Copy Markdown
Member

@Algunenano Algunenano left a comment

Choose a reason for hiding this comment

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

LGTM. Eventhough it's tagged as a breaking change so it gets a prominent place in the changelog, I think this should be backported. Lots of code relies on the fact that Strings are null terminated

@azat azat added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label May 26, 2025
@azat
Copy link
Copy Markdown
Member Author

azat commented May 26, 2025

Agree, added pr-must-backport

@azat
Copy link
Copy Markdown
Member Author

azat commented May 26, 2025

All CI failures are unrelated and will be fixed by #80386

@azat azat enabled auto-merge May 26, 2025 11:30
@azat
Copy link
Copy Markdown
Member Author

azat commented May 26, 2025

I think this should be backported. Lots of code relies on the fact that Strings are null terminated

BTW initially I did not add the pr-must-backport, since I was not sure about backporting such change, since this may cause issues after minor upgrades

@azat azat added this pull request to the merge queue May 26, 2025
Merged via the queue into ClickHouse:master with commit ab28be9 May 26, 2025
109 of 130 checks passed
@azat azat deleted the bitNot-String-NUL-terminated branch May 26, 2025 11:44
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-synced-to-cloud The PR is synced to the cloud repo label May 26, 2025
robot-ch-test-poll2 added a commit that referenced this pull request May 26, 2025
Cherry pick #80791 to 24.8: Fix bitNot() for String to return a zero-terminated string
robot-clickhouse added a commit that referenced this pull request May 26, 2025
robot-ch-test-poll2 added a commit that referenced this pull request May 26, 2025
Cherry pick #80791 to 25.3: Fix bitNot() for String to return a zero-terminated string
robot-clickhouse added a commit that referenced this pull request May 26, 2025
robot-ch-test-poll2 added a commit that referenced this pull request May 26, 2025
Cherry pick #80791 to 25.4: Fix bitNot() for String to return a zero-terminated string
robot-clickhouse added a commit that referenced this pull request May 26, 2025
robot-ch-test-poll2 added a commit that referenced this pull request May 26, 2025
Cherry pick #80791 to 25.5: Fix bitNot() for String to return a zero-terminated string
robot-clickhouse added a commit that referenced this pull request May 26, 2025
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created-cloud deprecated label, NOOP label May 26, 2025
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label May 26, 2025
Algunenano added a commit that referenced this pull request May 28, 2025
Backport #80791 to 25.5: Fix bitNot() for String to return a zero-terminated string
azat added a commit that referenced this pull request Jun 1, 2025
Backport #80791 to 25.4: Fix bitNot() for String to return a zero-terminated string
@alexey-milovidov alexey-milovidov changed the title Fix bitNot() for String to return a zero-terminated string Fix bitNot for String to return a zero-terminated string Jun 23, 2025
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-backward-incompatible Pull request with backwards incompatible changes pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'Logical error: 'res.data[res.size] == '\0'' (in StringValueCompatibility)

6 participants