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

Add warning, as and copied from BSON.jl. #613

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PallHaraldsson
Copy link

No description provided.

@PallHaraldsson
Copy link
Author

PallHaraldsson commented Nov 4, 2024

It seems good to let people know there (as soon as possible), but note at target:

https://github.com/JonasIsensee/JLD2DebugTools.jl is archived, so further changes may be needed to the docs, though I think it still works. Could be done in different (or same PR, if people want to suggest adding to it here soon).

@PallHaraldsson
Copy link
Author

I'm curious is the a non-default option to load safely, or could or should there be (i.e. by default)?

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.09%. Comparing base (fce896f) to head (d0d60e5).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
+ Coverage   85.08%   85.09%   +0.01%     
==========================================
  Files          37       37              
  Lines        4250     4247       -3     
==========================================
- Hits         3616     3614       -2     
+ Misses        634      633       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JonasIsensee
Copy link
Collaborator

JonasIsensee commented Nov 6, 2024

Hi!

Thank you for this suggestion.

Aside from people providing .jld2 files to me for debugging purposes I have not seen JLD2 files being distributed on the internet. That is part of the reason that there is no large warning at the top of the readme.
The other part is that this is a much more generic problem that exists for the serialization stdlib as well. (and any package installation...)

I suppose this also addresses your question on the default: The default is the "non-safe" version because otherwise (I would guess) 99% of users / use cases would need to explicitly add plain=false to their load calls.

Notes on the debug stuff should be removed. You are right.
The relevant features from JLD2DebugTools.jl were more or less added to base jld2 in #568
but since this provides a lot of experimental API, I have not really advertised it, yet.

@PallHaraldsson
Copy link
Author

The other part is that this is a much more generic problem that exists for the serialization stdlib as well. (and any package installation...)

Well that's interesting, I didn't know of this for stdlib Serialization; nor for Pkg installation, and is the latter a problem? It means you can run arbitrary code on installing a package (that shouldn't be needed), so is it a security bug? Since you likely plan to, and will end up using the package, then the same applies, so it's probably not considered a security issue.

Maybe the stdlib docs should warn if not already, but as for here if it isn't the default, as I thought, then can be closed here.

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.

2 participants