crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.22k stars 1.61k forks source link

`Crystal::System::User#from_*?` et al. don't work if required buffer size greater than initial buffer size #14619

Closed BlobCodes closed 2 weeks ago

BlobCodes commented 1 month ago

Bug Report

While looking at #14614, I noticed that all methods using System.retry_with_buffer do not work if the required buffer size to store strings in is greater than the initial buffer size provided.

Here's an example (Group#from_name?):

    grp = uninitialized LibC::Group
    grp_pointer = pointerof(grp)
    System.retry_with_buffer("getgrnam_r", GETGR_R_SIZE_MAX) do |buf|
      LibC.getgrnam_r(groupname, grp_pointer, buf, buf.size, pointerof(grp_pointer))
    end

LibC.getgrnam_r (and the other relevant methods) set the value of the last argument to NULL if an error occured:

On success, getgrnam_r() and getgrgid_r() return zero, and set result to grp. If no matching group record was found, these functions return 0 and store NULL in result. *In case of error, an error number is returned, and NULL is stored in result.**

However, the last argument provided is pointerof(grp_pointer), so grp_pointer will be set to null if the buffer was to small - but this variable is never reset and also used as the second argument to getgrnam_r (where the group data should be stored in).

A quick fix would be to replace the second argument (grp_pointer) with pointerof(grp), so the pointer is never null. With this fix, there's also no reason to define the value of grp_pointer, it could just be replaced by a out grp_pointer inside the function call.

TL;DR: After one iteration of retry_with_buffer, grp_pointer = null, which causes a segfault in very special circumstances.


With this bash script, you can get your very own segfault:

long_dir_name="/tmp/"
for ((i=0; i<20000; i++)); do long_dir_name+="a"; done

sudo useradd --base-dir="$long_dir_name" baduser

crystal eval '                                  
  require "system/user"
  System::User.find_by(name: "baduser")
'
straight-shoota commented 1 month ago

Oh yeah, the double use of grp_pointer as both input and output argument is really biting us here.