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

Pointer size not enforced #25

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

Conversation

schmurfy
Copy link
Contributor

@schmurfy schmurfy commented Jun 3, 2013

This is not really a pull request, more like an issue with a failing test as a start.
The problem is really simple, here is the failing test:

ptr2 = CFunc::UInt8[4]
CFunc::call(CFunc::Void, "strcpy", ptr2, "too long")
assert_equal "too ", ptr2.to_s

For me the problem is in the following code:

mrb_value
cfunc_pointer_to_s(mrb_state *mrb, mrb_value self)
{
    struct cfunc_type_data *data = DATA_PTR(self);
    size_t len;
    mrb_value str;
    struct RString *s;
    const char* p = (const char*)get_cfunc_pointer_data(data);


    len = strlen(p); // <-- problem

    str = mrb_str_new(mrb, 0, len);
    s = mrb_str_ptr(str);
    strcpy(s->ptr, p);
    s->len = strlen(s->ptr);
    return str;
}

When calling to_s on a pointer an strlen is done to check the string size, this is both wrong and dangerous and a good example of the reason why strncpy even exist...

While I know where the problem is I don't know what is the proper way to fix it, I think the pointer should store its allocated memory space and not try to go beyond its boundaries, in C if I did what the failing test does I would expect a segmentation fault but in this case I suppose the allocated memory space is inside a buffer reserved by mruby core and so instead of a segmentation fault this would just overwrite memory associated to another object/extension.

I noticed this problem when doing some tests with string arrays in structures.

@masuidrive
Copy link
Member

I understood this issue.
But I seem like It's C pointer issue.

How about in Ruby/DL library, do you know?

@schmurfy
Copy link
Contributor Author

schmurfy commented Jun 5, 2013

the ffi gem does ensure you cannot read or write outside of the allocated buffer, I think that is a sane way of doing this.

For me there are two different case, the first one (I will use C code but that's meant as pseudo code for cfunc):

char *c = some_func_returning_a_pointer();

In this case there is nothing to enforce as cfunc has no way to know the size of what is pointed, but in the second case:

char *c = malloc(1);
c[5] = 't';

This one is different since cfunc allocated the memory itself, that's this use case I was speaking about, if you do the same with the ffi gem it will raise an error saying that the index is out of bound and when you read it will never return more than the allocated size either.

@ppibburr
Copy link
Contributor

ppibburr commented Jun 7, 2013

Heres some code detailing this topic in MRI using DL and FFI

when you have a pointer you can write to wherever.
When it is a Struct, the two libraries handle out of bounds differnetly:

  • DL, just quietly drops when out of bounds
  • FFI, raises when out of bounds
require 'ffi'

module L 
  extend FFI::Library;
  ffi_lib "c";
  attach_function :malloc,[:int],:pointer; 
end;

class TStruct < FFI::Struct
  layout :name,[:uint8,5]
end

ptr = L.malloc(1)
ptr[7].write_uint8 65
ptr[7].read_uint8 == 65

# Raises index error
s = TStruct.new
[65,66,67,68,69,70,71].each_with_index do |b,i|
  s[:name][i] = b
end 
####

require 'dl'
require 'dl/import'
require 'dl/struct'

ptr = DL::CPtr.malloc(1)
ptr[0,7] = 'rambo !'
ptr.to_s == "rambo !"

# This quietly drops out of bounds
module Foo
    extend DL::Importer
    FOO = struct [ "unsigned int bar[5]" ]

    f = FOO.malloc
    f.bar = [65,66,67,68,69,70,71]
    p f.bar #=> [65,66,67,68,69]
end

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.

3 participants