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

Fix Base64#encode, exclude last 3 bytes from bswap #10752

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

kostya
Copy link
Contributor

@kostya kostya commented May 27, 2021

There is no specs, because I dont know how to really test guarded memory access, but test for various lengths percent 3 (from 0 to 7), already is: https://github.com/crystal-lang/crystal/blob/master/spec/std/base64_spec.cr#L7

… correct memory access), and process them manually
@kostya
Copy link
Contributor Author

kostya commented May 27, 2021

Also possible remove bswap at all, and use this code in main loop also, but this seems 7% slower.

@didactic-drunk
Copy link
Contributor

There is no specs, because I dont know how to really test guarded memory access, but test for various lengths percent 3 (from 0 to 7), already is: https://github.com/crystal-lang/crystal/blob/master/spec/std/base64_spec.cr#L7

If Mmap was part of std you could allocate your own guard page. The libc functions are already mapped. It just needs a small wrapper class. Is this something that would be accepted as a PR? @asterite

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text labels May 28, 2021
@straight-shoota straight-shoota linked an issue May 28, 2021 that may be closed by this pull request
@straight-shoota straight-shoota changed the title fixed #10751, Base64#encode, exclude last 3 bytes from bswap Fix Base64#encode, exclude last 3 bytes from bswap May 28, 2021
@asterite
Copy link
Member

I think this is fine, thanks!

@didactic-drunk we already use mmap for fibers, so that's already a thing, though you have to use LibC.mmap I guess. Is that to allocate memory for the string that, if outside of range, will error?

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@didactic-drunk
Copy link
Contributor

@asterite Today's purpose is allocatong a guard page after the Slice and probably the Slice (Does an unallocated memory range exist after a random String? Probably not)

A general purpose Mmap class would at minimum hold the raw pointer and contain methods for changing protection levels, remapping, resizing, syncing and to_slice. I could use it to make Sodium like allocated memory regions with guard pages on each side without a Sodium dependency similar to memguard LockedBuffer. I have a much longer feature request on secret handling that requires this. The post is still WIP and may finish today.

This shard uses mmap to map files and would also benefit from a general purpose Mmap class.

@didactic-drunk
Copy link
Contributor

Never mind. I'm withdrawing my mmap request. I'll make a shard and if necessary you can import it later.

@asterite
Copy link
Member

Oh, I think I understand now what you mean by protrcting the heap. But we use boehm GC for allocating memory, I'm not sure how we can also use mmap. Plus string literals use ROM data. So I'm not sure it can help in this case. But I know nothing about mmap.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @kostya 🙏

@straight-shoota straight-shoota added this to the 1.1.0 milestone May 29, 2021
@didactic-drunk
Copy link
Contributor

didactic-drunk commented May 29, 2021

@asterite I'm not suggesting protecting the heap, but mmap block of memory and mmap a PROT_NONE guard page after it. Then provide the first mmap block as a Slice to Base64.

Pseudo code

size = 48
writable_page = mmap(starting_address, 4096, PROT_READ | PROT_WRITE)
guard_page = mmap(starting_address + 4096, 4096, PROT_NONE)
writable_addr = writable_page + (4096 - size)
slice = Slice.new writable_addr, size
Base64.encode slice

@straight-shoota straight-shoota merged commit fc8ae1e into crystal-lang:master Jun 2, 2021
@straight-shoota
Copy link
Member

Thanks, @kostya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base64 references memory past end of Slice
5 participants