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

Buffer Overflow during precompile #189

Open
Zentrik opened this issue Apr 4, 2024 · 4 comments · May be fixed by #193
Open

Buffer Overflow during precompile #189

Zentrik opened this issue Apr 4, 2024 · 4 comments · May be fixed by #193

Comments

@Zentrik
Copy link

Zentrik commented Apr 4, 2024

Specifically, it seems to be the call to mpfr_strtofr added in 504dfdf.
See JuliaLang/julia#53898 for more info.

https://gitlab.inria.fr/mpfr/mpfr/-/blob/4.2.0/src/strtofr.c?ref_type=tags#L326 is the line where the error occurs,

pstr->alloc = (size_t) strlen (str) + 1;

Are we missing the null terminator?

@Zentrik
Copy link
Author

Zentrik commented Apr 4, 2024

Minimal reproducer: Parsers.xparse(BigFloat, Vector(codeunits("1")), 1, 1).

@Zentrik
Copy link
Author

Zentrik commented Apr 5, 2024

Seems like we should also be using Cstring instead of Ptr as the second argument type to mpfr_strtofr.

@Zentrik Zentrik changed the title ASAN throwing heap-buffer-overflow errors during precompilation Buffer Overflow during precompile Apr 5, 2024
@quinnj
Copy link
Member

quinnj commented Apr 6, 2024

Hmmmm, interesting. Yes, I've definitely been assuming that a String or Vector{UInt8} input source will have the implicit nul terminator byte already there for us. Do you happen to know if this is a new failure? Like, since the Memory changes in Base? Doe we not see a Buffer Overflow in 1.9? That would suggest that maybe something with the new Memory changes is not the same as before.

Does just changing teh ccall argument from Ptr{UInt8} to Cstring fix the ASAN warning? I believe that should ensure the NUL terminator byte is present no matter the input, so if that fixes it, then that sounds like a fine change to me.

@Zentrik
Copy link
Author

Zentrik commented Apr 6, 2024

for i in 1:10^7
    a = Vector(codeunits("123"))
    ptr = pointer(a)
    Base.unsafe_load(ptr)
    Base.unsafe_load(ptr+1)
    Base.unsafe_load(ptr+2)
    if Base.unsafe_load(ptr+3) != 0
        return i
    end
end

returns nothing on 1.10.2 whilst on 1.11-alpha2 it returns numbers below 100.
So perhaps something has changed, though I don't think it's guaranteed that a vector has a null terminator.

I'm having a little trouble with my asan build right now, but I see this on 1.11-alpha2 so I don't think just changing Ptr to Cstring is going to work.

julia> a = for i in 1:10^7
           a = Vector(codeunits("123"))
           ptr = pointer(a)
           Base.unsafe_load(ptr)
           Base.unsafe_load(ptr+1)
           Base.unsafe_load(ptr+2)
           if Base.unsafe_load(ptr+3) != 0
               return a
           end
       end
3-element Vector{UInt8}:
 0x31
 0x32
 0x33

julia> unsafe_string(Base.cconvert(Cstring, pointer(a)))
"123\xec\xff\x7f"

Oddly, there is no cconvert(Cstring, Vector) even though there is one for Cwstring.
I think a safe thing to do is to just copy into a String and then change Ptr to Cstring as well for good measure.

@Zentrik Zentrik linked a pull request Oct 26, 2024 that will close this issue
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.

2 participants