Skip to content
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.

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

Hermes: fix native stack checking on Android/Bionic #588

Closed
tmikov opened this issue Oct 18, 2024 · 8 comments
Closed

Hermes: fix native stack checking on Android/Bionic #588

tmikov opened this issue Oct 18, 2024 · 8 comments

Comments

@tmikov
Copy link

tmikov commented Oct 18, 2024

Target Branch(es)

0.76, 0.75

Link to commit or PR to be picked

facebook/hermes@a5b2bbc

Description

Fixes stack overflow checking when the guard size is 64KB. This is particularly important for security-oriented distributions like GrapheneOS, that are maintaining binary compatibility with Android. They flagged this to us, and I think it is a good idea to pick it.

(I think I am able to pick this myself into Hermes branches, but I don't want to disturb the official process. However if it would make it easier...)

@joe-sam
Copy link

joe-sam commented Oct 18, 2024

You probably need to file separate pick requests for each of the branch version, and definitely prefix them with a square bracket e.g. [0.7x.] so that the pick is automatically sorted and elevated to notice of the correct project thread/team.

@cortinico
Copy link
Collaborator

Also this won't make it to 0.76.0 and will land in 0.76.1

@tmikov
Copy link
Author

tmikov commented Oct 18, 2024

OK, so close this and create new requests? I suggest fixing the issue template - there was no indication that square brackets need to be used.

@tmikov tmikov closed this as completed Oct 18, 2024
@joe-sam
Copy link

joe-sam commented Oct 21, 2024

there was no indication that square brackets need to be used.

Probably not really necessary and just for human readability and convenience.
The hermes engines are really long overdue to be bumped anyway, due to JSI header divergence, but I understand this is not at all trivial exercise. an automated workflow every time this happens would be helpful ?

@tmikov
Copy link
Author

tmikov commented Oct 21, 2024

@joe-sam can you elaborate on the divergence? RN and Hermes/Static Hermes are always synchronized in trunk and in every branch cut.

@joe-sam
Copy link

joe-sam commented Oct 21, 2024

@tmikov have already elaborated on the divergence.

#579 (comment)

AFAIK from the git diffs compare tags attached to the issue, the "required" hermes engine bump did not occur on the previous changes to the JSI header after the previous cherry-pick
Add missing withRuntimeDecorator
There does not appear to be an intermediate hermes release tag after the changes to the JSI as I would expect.
The next tag containing the correct headers appears to be aligned to the 0.76 release candidate instead of 0.75.

@tmikov
Copy link
Author

tmikov commented Oct 22, 2024

@joe-sam sorry, I don't understand, probably because I am not very familiar with the RN release process. I am not sure what you mean by a "bump" of Hermes.

Whenever RN cuts a release, it tags the current (trunk) version of Hermes. As a rule, JSI header changes should only occur at those points, never in cherry-picks (I suppose exceptions can be made for critical bugs).

@joe-sam
Copy link

joe-sam commented Oct 22, 2024

@tmikov I am also not familiar with the release process, but that is also what I would expect.

Whenever RN cuts a release, it tags the current (trunk) version of Hermes. As a rule, JSI header changes should only occur at those points, never in cherry-picks (I suppose exceptions can be made for critical bugs).

The tags don't correspond to a semantic versioning scheme( MAJOR.MINOR.PATCH) (but they do at least have dates + corresponding RN numbers which I refer to the order of releases) so I don't know what you would refer to a patch release, but i am referring to a bump in the patch number.
I am only making an effort pointing out the synced up trail of git logs JSI vis-a-vis RN/Hermes, and where it doesn't match to what was expected. That is to say - either hermes version was "bumped" up to a patch release in the previous cherry pick containing matching headers and was not tagged which doesn't make sense or more likely it was not bumped - there was no patch release of hermes corresponding to the JSI changes in the cherry pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants