Skip to content

Make C25 fix for build ids containing a zero byte - #5480

Merged
hlwarriner merged 2 commits into
youtube:25.lts.1+from
hlwarriner:buildid-fix-25
Apr 23, 2025
Merged

Make C25 fix for build ids containing a zero byte#5480
hlwarriner merged 2 commits into
youtube:25.lts.1+from
hlwarriner:buildid-fix-25

Conversation

@hlwarriner

@hlwarriner hlwarriner commented Apr 21, 2025

Copy link
Copy Markdown
Member

This change replaces strlcpy with memcpy for copying build ids. As described in the bug, a byte array representing a build id should not be treated as a string; it may contain zero bytes (0x00) and these should not be considered null terminators.

Changing the type of build_id in the EvergreenInfo struct from char array to uint8_t would help avoid these types of bugs. But I would rather save that change for C26.

b/408471930

Change-Id: I02cddaf01c47b0883d21997c36ea0c9d1c8ca40c

@hlwarriner
hlwarriner requested review from y4vor and yell0wd0g April 21, 2025 17:43
@hlwarriner

hlwarriner commented Apr 21, 2025

Copy link
Copy Markdown
Member Author

I looked into adding a unit test for PublishEvergreenInfo for this edge case but we would need to either:

  1. Manually create an ELF note section with a stub build id, which looks pretty cumbersome.
  2. Add some test-only method to ProgramTable so we can manually set a stub build_id_ value in tests, which I'd like to avoid.
  3. Significantly redesign the class under test.

I'll think more about this - and suggestions are welcome - but I don't think we should block the fix on the unit test. I did do some manual end-to-end testing, for example in https://b.corp.google.com/issues/408471930#comment5.

yell0wd0g
yell0wd0g approved these changes Apr 21, 2025
View reviewed changes
Comment thread starboard/elf_loader/program_table.cc Resolved
Comment thread starboard/elf_loader/program_table.cc Resolved
@hlwarriner
hlwarriner requested a review from yell0wd0g April 21, 2025 19:27
@hlwarriner
Make C25 fix for build ids containing a zero byte
aee3a37 
This change replaces strlcpy with memcpy for copying build ids. As
described in the bug, a byte array representing a build id should not be
treated as a string; it may contain zero bytes (0x00) and these should
not be considered null terminators.

Changing the type of build_id in the EvergreenInfo struct from char array
to uint8_t would help avoid these types of bugs. But I would rather save
that change for C26.

b/408471930

Change-Id: I02cddaf01c47b0883d21997c36ea0c9d1c8ca40c
@hlwarriner
Add build id size check before memcpy
04269df 
Change-Id: Ib6cf07f9cda66a055514b4bce53424e25adaa0c6
@hlwarriner

hlwarriner commented Apr 21, 2025

Copy link
Copy Markdown
Member Author

I believe the failing checks are all from b/255654807 and unrelated to this change.

hlwarriner added a commit to hlwarriner/cobalt that referenced this pull request Apr 22, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte
af38b02 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
@hlwarriner hlwarriner mentioned this pull request Apr 22, 2025
Merged
@hlwarriner
hlwarriner merged commit 3ccdb04 into youtube:25.lts.1+ Apr 23, 2025
hlwarriner added a commit to hlwarriner/cobalt that referenced this pull request Apr 23, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte
33e403a 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
hlwarriner added a commit that referenced this pull request Apr 23, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte (#5502)
cb7ef3a 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
kaidokert pushed a commit to kaidokert/cobalt that referenced this pull request Jun 15, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte (youtube#5502)
35357e0 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
briantting pushed a commit that referenced this pull request Jul 1, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte (#5502)
9c97c7f 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a
briantting pushed a commit that referenced this pull request Jul 11, 2025
@hlwarriner @briantting
Fwd port fix for build ids containing a zero byte (#5502)
beb8289 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a
sideb0ard pushed a commit to sideb0ard/cobalt that referenced this pull request Jul 25, 2025
@hlwarriner
[CONFLICT] Original Commit: 0c4845214d46ed8fc65519cf08c6b27bc58676a6
be35151 
Original Message:
Fwd port fix for build ids containing a zero byte (youtube#5502)

This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a


Files with conflicts:
- starboard/elf_loader/evergreen_info.h
sideb0ard pushed a commit to sideb0ard/cobalt that referenced this pull request Jul 26, 2025
@hlwarriner @sideb0ard
Fwd port fix for build ids containing a zero byte (youtube#5502)
85e9bbe 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a
sideb0ard pushed a commit to sideb0ard/cobalt that referenced this pull request Jul 30, 2025
@hlwarriner @sideb0ard
Fwd port fix for build ids containing a zero byte (youtube#5502)
e5c71c0 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a
sideb0ard pushed a commit that referenced this pull request Aug 15, 2025
@hlwarriner @sideb0ard
Fwd port fix for build ids containing a zero byte (#5502)
7cb080f 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930
original-hexsha: cb7ef3a
briantting pushed a commit that referenced this pull request Aug 15, 2025
@hlwarriner
Fwd port fix for build ids containing a zero byte (#5502)
c53a348 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930

original-hexsha: cb7ef3a
sideb0ard pushed a commit that referenced this pull request Aug 20, 2025
@hlwarriner @sideb0ard
Fwd port fix for build ids containing a zero byte (#5502)
b7020d8 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930

original-hexsha: cb7ef3a
briantting pushed a commit that referenced this pull request Aug 29, 2025
@hlwarriner @briantting
Fwd port fix for build ids containing a zero byte (#5502)
2bbb538 
This change is based on #5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930

original-hexsha: cb7ef3a
madhurajayaraman pushed a commit to madhurajayaraman/cobalt that referenced this pull request Sep 9, 2025
@hlwarriner @madhurajayaraman
Fwd port fix for build ids containing a zero byte (youtube#5502)
76ed866 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930

original-hexsha: cb7ef3a
sideb0ard pushed a commit to sideb0ard/cobalt that referenced this pull request Sep 10, 2025
@hlwarriner @sideb0ard
Fwd port fix for build ids containing a zero byte (youtube#5502)
45824ff 
This change is based on youtube#5480, which makes this fix on the 25.lts.1+
branch. This current change goes a step further by changing the type of
build_id in the EvergreenInfo struct; a uint8_t array should better
represent the data and provide some type safety.

b/408471930

original-hexsha: cb7ef3a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@y4vor y4vor y4vor approved these changes
@yell0wd0g yell0wd0g Awaiting requested review from yell0wd0g

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hlwarriner @y4vor @yell0wd0g