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.cstring() fails to null-terminate a trimmed string under certain circumstances. #2613

Closed
mfelsche opened this issue Mar 26, 2018 · 2 comments

Comments

@mfelsche
Copy link
Contributor

This bug happened to me, when extracting the port from a command line arg like localhost:9999:A using String.trim and passing this to TCPListener.create. It failed to start listening, turned out the port/service cstring which should have been 9999, which was a shared slice of the whole localhost:9999:A, was actually: 9999:A, because String.cstring() failed to null-terminate the char* properly.

This happened to me on both of these systems, both compiling in debug and release mode:

  • Linux 4.13.0-37-generic #42~16.04.1-Ubuntu SMP Wed Mar 7 16:03:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

    0.21.3-fb10149 [debug]
    compiled with: llvm 3.9.1 -- cc (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
    
  • Linux 4.14.24 #1-NixOS SMP Sat Mar 3 09:24:39 UTC 2018 x86_64 GNU/Linux

    0.21.3-98bbbbb5 [debug]
    compiled with: llvm 5.0.0 -- gcc (GCC) 6.4.0
    

Code to reproduce: https://gist.github.com/mfelsche/6b377d9aa83eb45c1307a5ecec37447f
If it prints . everything is fine, if it prints and E, the string was not properly null-terminated.

# compiled in release mode
$ while true; do ./scratch -a localhost:9999:A && sleep 0.1; done
........E..............E.EEE.......E................E...E.E.E..E...E.E.EE.E.E...EE...E....E.E.....EE.^C%  
# compiled in debug mode
 while true; do ./scratch -a localhost:9999:A && sleep 0.1; done
...E..EEEE.EEEEE.EE..E.EE.EEEEEEE.EE.EEEEE.E.EEEEEE.EEEEEEE.EE.EEEEEEE.EEEEEEE.EEEEEEEE.EEEEEEE..E.EEEE.EE^C% 

I was not able to reproduce this in any other way. The traces of the string in question come from a cloned Array[String] that is shift()?ed until nothing is left. The string is then trimmed to cut out the part between the colons. Nothing super-special. Puzzling.

@mfelsche
Copy link
Contributor Author

Here is a snippet of an lldb session which reveals what happens in String.cstring(). It copies the contents of the trimmed string properly to that pointer, but fails to do Pointer[U8]._update(_size + 1, 0) somehow.

Process 10154 stopped
* thread #2, name = 'scratch', stop reason = step over
    frame #0: 0x0000000000458e21 scratch`String_val_cstring_o(this=0x00007fffe54c5ce0) at string.pony:229
   226 	    is copied into a new, null-terminated allocation.
   227 	    """
   228 	    if is_null_terminated() then
-> 229 	      return _ptr
   230 	    end
   231 	
   232 	    let ptr = Pointer[U8]._alloc(_size + 1)
(lldb) p (char*)this->_ptr
(char *) $3 = 0x00007ffff6cb44ca "9999:A"
(lldb) p (char*)ptr
(char *) $4 = 0x00000000004921b8 "\xffffffa0\xffffffb3H"
(lldb) n
Process 10154 stopped
* thread #2, name = 'scratch', stop reason = step over
    frame #0: 0x0000000000458e2f scratch`String_val_cstring_o(this=0x00007fffe54c5ce0) at string.pony:232
   229 	      return _ptr
   230 	    end
   231 	
-> 232 	    let ptr = Pointer[U8]._alloc(_size + 1)
   233 	    _ptr._copy_to(ptr._unsafe(), _size)
   234 	    ptr._update(_size + 1, 0)
   235 	    ptr
(lldb) p (char*)ptr
(char *) $5 = 0x00000000004921b8 "\xffffffa0\xffffffb3H"
(lldb) n
Process 10154 stopped
* thread #2, name = 'scratch', stop reason = step over
    frame #0: 0x0000000000458e4e scratch`String_val_cstring_o(this=0x00007fffe54c5ce0) at string.pony:233
   230 	    end
   231 	
   232 	    let ptr = Pointer[U8]._alloc(_size + 1)
-> 233 	    _ptr._copy_to(ptr._unsafe(), _size)
   234 	    ptr._update(_size + 1, 0)
   235 	    ptr
   236 	
(lldb) p (char*)ptr
(char *) $6 = 0x00007fffe54d1400 <no value available>
(lldb) n
Process 10154 stopped
* thread #2, name = 'scratch', stop reason = step over
    frame #0: 0x0000000000458e6a scratch`String_val_cstring_o(this=0x00007fffe54c5ce0) at string.pony:234
   231 	
   232 	    let ptr = Pointer[U8]._alloc(_size + 1)
   233 	    _ptr._copy_to(ptr._unsafe(), _size)
-> 234 	    ptr._update(_size + 1, 0)
   235 	    ptr
   236 	
   237 	  fun val array(): Array[U8] val =>
(lldb) p (char*)ptr
(char *) $7 = 0x00007fffe54d1400 "9999\xffffffff\x7f"
(lldb) n
Process 10154 stopped
* thread #2, name = 'scratch', stop reason = step over
    frame #0: 0x0000000000458e8b scratch`String_val_cstring_o(this=0x00007fffe54c5ce0) at string.pony:235
   232 	    let ptr = Pointer[U8]._alloc(_size + 1)
   233 	    _ptr._copy_to(ptr._unsafe(), _size)
   234 	    ptr._update(_size + 1, 0)
-> 235 	    ptr
   236 	
   237 	  fun val array(): Array[U8] val =>
   238 	    """
(lldb) p (char*)ptr
(char *) $8 = 0x00007fffe54d1400 "9999\xffffffff"

@mfelsche
Copy link
Contributor Author

It looks like this is a simple off-by-one issue, which only affects cases where the memory location being returned from alloc has already been used. it should be ptr._update(_size, 0)

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

No branches or pull requests

1 participant