getresuid and getresgid in sb-posix are borked. Always causes memory fault

Bug #1603806 reported by Kieran Grant on 2016-07-17
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
SBCL
Undecided
Unassigned

Bug Description

getresuid and getresgid function in the sb-contrib modules are incorrectly defined.
They are meant to be:
int getresuid(uid_t *ruid, uid_t *euid, uid_t *suid)
int getresgid(gid_t *rgid, gid_t *egid, gid_t *sgid)
but sb-contrib things they are:
uid_t getresuid(void)
git_d getresgid(void)

This results in a Memory fault error...
One may be able to continue, but the system has probably trashed stuff that was pointed to on the stack... :/
How to verify:
(require :sb-posix)
(sb-posix:getresuid)
OR
(require :sb-posix)
(sb-posix:getresgid)

Tested on both latest stable 1.3.7 and on git master.
Just to meet guidelines here is my info:
Linux kieran-desktop 4.4.0-31-generic #50-Ubuntu SMP Wed Jul 13 00:07:12 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
*features*:
(:64-BIT :64-BIT-REGISTERS :ALIEN-CALLBACKS :ANSI-CL :ASH-RIGHT-VOPS
 :C-STACK-IS-CONTROL-STACK :COMMON-LISP :COMPARE-AND-SWAP-VOPS
 :COMPLEX-FLOAT-VOPS :CYCLE-COUNTER :ELF :FLOAT-EQL-VOPS
 :FP-AND-PC-STANDARD-SAVE :GENCGC :IEEE-FLOATING-POINT :INLINE-CONSTANTS
 :INTEGER-EQL-VOP :LARGEFILE :LINKAGE-TABLE :LINUX :LITTLE-ENDIAN
 :MEMORY-BARRIER-VOPS :MULTIPLY-HIGH-VOPS :OS-PROVIDES-BLKSIZE-T
 :OS-PROVIDES-DLADDR :OS-PROVIDES-DLOPEN :OS-PROVIDES-GETPROTOBY-R
 :OS-PROVIDES-POLL :OS-PROVIDES-PUTWC :OS-PROVIDES-SUSECONDS-T
 :PACKAGE-LOCAL-NICKNAMES :PRECISE-ARG-COUNT-ERROR :RAW-INSTANCE-INIT-VOPS
 :SB-AFTER-XC-CORE :SB-CORE-COMPRESSION :SB-DOC :SB-EVAL :SB-FUTEX :SB-LDB
 :SB-PACKAGE-LOCKS :SB-SIMD-PACK :SB-SOURCE-LOCATIONS :SB-TEST :SB-THREAD
 :SB-UNICODE :SB-XREF-FOR-INTERNALS :SBCL :STACK-ALLOCATABLE-CLOSURES
 :STACK-ALLOCATABLE-FIXED-OBJECTS :STACK-ALLOCATABLE-LISTS
 :STACK-ALLOCATABLE-VECTORS :STACK-GROWS-DOWNWARD-NOT-UPWARD :SYMBOL-INFO-VOPS
 :UNIX :UNWIND-TO-FRAME-AND-CALL-VOP :X86-64)

I'll attach a patch. To use the getresuid and getresgid with this patch use must use the likes of with-alien like so:
(require :sb-posix)
(with-alien ((ruid sb-posix::uid-t) (euid sb-posix::uid-t) (suid sb-posix::uid-t))
(sb-posix:getresuid (addr ruid) (addr euid) (addr suid))
(values ruid euid suid))

Attila Lendvai (attila-lendvai) wrote :

returns 0 without a crash on sbcl-1.2.4.debian-linux-x64

but it crashes 1.3.4.104.hu.dwim.4-a36628d (x64)

Kieran Grant (kieran-grant) wrote :

Are you getting a single 0?
I checked 1.2.4 sources, it hasn't changed the getresuid and getresgid code.
The C functions getresuid and getresgid return 0 on success, -1 on error.
This means that sbcl-1.2.4.debian-linux-x64 version didn't notice it's stack being trashed.
This is because sb-posix needs to pass pointers to 3 ints that the C function (really, the Kernel) sets, but if the address are invalid, you get an error.
Because it was declared "never-fails" SBCL always returns an integer result.
(In this case, either 0 for success or -1 for invalid address).
On recent SBCL's, it would notice it's stack being nuked.

Attila Lendvai (attila-lendvai) wrote :

it's probably random. the crash probably depends on the actual memory layout. didn't try repeatedly, just a quick copy-paste.

Stas Boukarev (stassats) wrote :

Fixed in a833d4df8373d42ea359ba0e667cb56ed66b55fc with a more appropriate fix.

Changed in sbcl:
status: New → Fix Committed
Kieran Grant (kieran-grant) wrote :

Woot!
Yeah... I suppose that it a better and more lispy approach.
I was unsure how "low-level" this line in the sb-posix readme meant:
"The general intent is for a low-level interface."
The original fix I was going to send did the same thing... but the "C" in me made the user have to pass Alien pointers...
Oh well, that's what you get from somebody still half stuck in a C mindset trying to transition to a LISP mindset... :D I'll get there one day :D

Stas Boukarev (stassats) on 2016-08-02
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers