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

String constructor on a view can mutate parent #54156

Closed
nhz2 opened this issue Apr 19, 2024 · 8 comments
Closed

String constructor on a view can mutate parent #54156

nhz2 opened this issue Apr 19, 2024 · 8 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
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
Member

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

@vtjnash
Copy link
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
Copy link
Contributor Author

nhz2 commented Jul 31, 2024

Fixed by #54927

@nhz2 nhz2 closed this as completed Jul 31, 2024
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 a pull request may close this issue.

4 participants