Skip to content

ORC-1834: [C++] Fix undefined behavior#2112

Closed
georgthegreat wants to merge 1 commit intoapache:mainfrom
georgthegreat:patch-3
Closed

ORC-1834: [C++] Fix undefined behavior#2112
georgthegreat wants to merge 1 commit intoapache:mainfrom
georgthegreat:patch-3

Conversation

@georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented Jan 13, 2025

What changes were proposed in this pull request?

Unaligned reads are UB in C++, memcpy-ing zero bytes is UB either.

How was this patch tested?

Internal UBsan report was used to detect and fix this bug.

@github-actions github-actions bot added the CPP label Jan 13, 2025
@georgthegreat
Copy link
Contributor Author

georgthegreat commented Jan 13, 2025

The form suggest filing a JIRA issue, but

Public signup for this instance is disabled.

@wgtmac
Copy link
Member

wgtmac commented Jan 13, 2025

Thanks for fixing this!

Public signup for this instance is disabled.

You may request an account at https://selfserve.apache.org/jira-account.html

@georgthegreat georgthegreat changed the title Fix undefined behavior ORC-1834: Fix undefined behavior Jan 13, 2025
@georgthegreat
Copy link
Contributor Author

georgthegreat commented Jan 13, 2025

@wgtmac, I have filed https://issues.apache.org/jira/browse/ORC-1834

dongjoon-hyun
dongjoon-hyun reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for making a PR, @georgthegreat .

I re-triggered CI after merging the following.

@georgthegreat @wgtmac
Fix undefined behavior
ba5f5bc 
Unaligned reads are UB in C++, memcpy-ing zero bytes is UB either.
@wgtmac
Copy link
Member

wgtmac commented Jan 14, 2025

Thanks @dongjoon-hyun! The linter CI is green after rebased: https://github.com/apache/orc/actions/runs/12759485419/job/35563347180?pr=2112

@wgtmac wgtmac changed the title ORC-1834: Fix undefined behavior ORC-1834: [C++] Fix undefined behavior Jan 14, 2025
@wgtmac
Copy link
Member

wgtmac commented Jan 14, 2025

Perhaps we should enable UBSAN together with the ASAN CI.

@wgtmac wgtmac closed this in ab084b5 Jan 16, 2025
@dongjoon-hyun dongjoon-hyun added this to the 2.2.0 milestone Jan 16, 2025
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 16, 2025

Shall we backport this bug fix to branch-2.1 ? Or, Apache ORC 2.2 (2026-01-15) is enough for delivery schedule?

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Jan 16, 2025

For us it would be better to have this backported to 2.1 and have a release tagged.
Thus we could remove the patch from the internal version.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 17, 2025

Got it, @georgthegreat .

dongjoon-hyun pushed a commit that referenced this pull request Jan 17, 2025
@georgthegreat @dongjoon-hyun
ORC-1834: [C++] Fix undefined behavior
e0fee1e 
### What changes were proposed in this pull request?
Unaligned reads are UB in C++, memcpy-ing zero bytes is UB either.

### How was this patch tested?
Internal UBsan report was used to detect and fix this bug.

Closes #2112 from georgthegreat/patch-3.

Authored-by: Yuriy Chernyshov <thegeorg@yandex-team.com>
Signed-off-by: Gang Wu <ustcwg@gmail.com>
(cherry picked from commit ab084b5)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun dongjoon-hyun modified the milestones: 2.2.0, 2.1.1 Jan 17, 2025
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 17, 2025 •
edited
Loading

I backported this to branch-2.1 and reassigned the milestone to 2.1.1.

@dongjoon-hyun dongjoon-hyun modified the milestones: 2.1.1, 2.2.0 Jan 17, 2025
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 17, 2025 •
edited
Loading

My bad. I removed it from branch-2.1 because it caused CPP linter CI failures.

Screenshot 2025-01-16 at 17 53 38

In this case, we need a backporting PR. Could you make another PR to branch-2.1, @georgthegreat ?

georgthegreat added a commit to georgthegreat/orc that referenced this pull request Jan 17, 2025
@georgthegreat
Backport apache#2112 to 2.1 branch
d0102ea
@georgthegreat georgthegreat mentioned this pull request Jan 17, 2025
Closed
wgtmac pushed a commit to georgthegreat/orc that referenced this pull request Jan 22, 2025
@georgthegreat @wgtmac
Backport apache#2112 to 2.1 branch
9c53c5b
wgtmac pushed a commit that referenced this pull request Jan 22, 2025
@georgthegreat @wgtmac
ORC-1834: [C++] Fix undefined behavior
825f248 
See #2112

Closes #2116 from georgthegreat/patch-4.

Authored-by: Yuriy Chernyshov <thegeorg@yandex-team.com>
Signed-off-by: Gang Wu <ustcwg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@wgtmac wgtmac wgtmac approved these changes

Assignees

No one assigned

Labels

CPP

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@georgthegreat @wgtmac @dongjoon-hyun