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.

Sign up for GitHub

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

Jump to bottom

String constructor on a view can mutate parent #54156

Open
nhz2 opened this issue Apr 19, 2024 · 7 comments
Open

String constructor on a view can mutate parent #54156

nhz2 opened this issue Apr 19, 2024 · 7 comments

Comments

@nhz2
Copy link
Contributor

nhz2 commented Apr 19, 2024

This has happened since #53896

Here is a MWE.

julia> a = Memory{UInt8}(undef, 10);

julia> fill!(a, 0x01);

julia> String(view(a, 1:2))
"\x01\x01"

julia> a
0-element Memory{UInt8}

I expected a to not be changed.

This is causing issues in JuliaWeb/HTTP.jl#1166
And I am also using String(view( in https://github.com/JuliaIO/ZipArchives.jl/blob/5bacade2f71e0c28e2aeba09d21a665ecad131cd/src/reader.jl#L106
@adienes
Copy link
Contributor

adienes commented Apr 19, 2024

I believe this is intended

see also: #32528

@nhz2
Copy link
Contributor Author

nhz2 commented Apr 19, 2024

The issue is that view protects the parent from being truncated by String in 1.10, but in 1.11.0-beta1 this is broken.
If this is intended what is the correct way to protect the parent from being truncated?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 19, 2024

In older versions, the view function also enforced a copy, which seems awkward to expect there. We should possibly check though that the truncation of the Memory object only applies if it is backed by a String (and doesn't make a copy)?

@nhz2
Copy link
Contributor Author

nhz2 commented Apr 22, 2024

Maybe the view method that converts a Memory to a Vector could be called something like make_vector to avoid confusion with normal view? The difference would be that the result of view would have a fixed size, and create a copy when passed to String.

@KristofferC
Copy link
Sponsor Member

Maybe #53896 should be reverted / rethought in light of this.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 22, 2024

We also possibly should not unsafely mutate this Memory in this case. That is somewhat more of an Array feature anyways that you should possibly lose when accessing Memory directly

@nhz2
Copy link
Contributor Author

nhz2 commented Apr 24, 2024

If I understand:

julia/src/genericmemory.c

Lines 196 to 207 in c38e7cd

m->length = 0;
if (how != 0) {
jl_value_t *o = jl_genericmemory_data_owner_field(m);
jl_genericmemory_data_owner_field(m) = NULL;
if (how == 3 &&
((mlength + sizeof(void*) + 1 <= GC_MAX_SZCLASS) == (len + sizeof(void*) + 1 <= GC_MAX_SZCLASS))) {
if (jl_string_data(o)[len] != '\0')
jl_string_data(o)[len] = '\0';
if (*(size_t*)o != len)
*(size_t*)o = len;
return o;
}
correctly, there are two possible mutations, setting length to 0, and setting the byte after the end to null. Are String required to always be null terminated?

@nhz2 nhz2 mentioned this issue Apr 26, 2024
Closed
@oscardssmith oscardssmith mentioned this issue May 9, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rename Memory wrapper from view to unsafe_vector nhz2/julia
4 participants
@vtjnash @KristofferC @nhz2 @adienes