Skip to content

Conversation

@timholy
Copy link
Member

@timholy timholy commented Feb 28, 2023

Fixes #48354

(I should say I haven't yet tested that it actually fixes it, but given the diagnosis it seems very likely. Can test tomorrow if no one beats me to it.)

@DilumAluthge
Copy link
Member

Backport to 1.9?

@timholy timholy added the backport 1.9 Change should be backported to release-1.9 label Feb 28, 2023
@vtjnash
Copy link
Member

vtjnash commented Feb 28, 2023

I don't think this will be correct or reliable. We already should have checked this in a more reliable way elsewhere. What we did not check is the checksum value matches (we check the pkgimages, but specifically exclude the sysimg from that test by setting it to zero)

@vchuravy
Copy link
Member

Hm... I thought we were already doing this. This is a fine strategy, but I would prefer Base just being a normal package in the dependent module list that we verify

@timholy
Copy link
Member Author

timholy commented Mar 1, 2023

Hmm, actually Base is listed in the depmods, so indeed the build_id and uuid are already checked.

What we did not check is the checksum value matches (we check the pkgimages, but specifically exclude the sysimg from that test by setting it to zero)

@vtjnash, can you link to where we set it to zero? AFAICT the issue is that we don't check the checksum of each depmod:
https://github.com/JuliaLang/julia/blob/master/src/staticdata_utils.c#L1145-L1159

and the check at package-loading time only checks the checksum of the package itself:

checksum = read_uint64(s);

So is the right fix to start encoding the sysimage checksum in each package header? I'm not certain I understand what your comment really means, hoping to clarify my understanding of what you mean.

@vtjnash
Copy link
Member

vtjnash commented Mar 1, 2023

Those build_ids are actually checksums for the pkgimag, but we don't load the checksum for the sysimg into those.

@timholy
Copy link
Member Author

timholy commented Mar 1, 2023

I see, so it sounds like the right fix is to stop using 0 as the build_id for the sysimage. I can make that change, hopefully by tomorrow.

@DilumAluthge DilumAluthge deleted the teh/check_build_ids branch March 3, 2023 04:53
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1.9.0-beta3 segfaults when loading Revise into a session using a custom sysimage

6 participants