From 621f8a0178d2f66470ca978716b4a62f33f2dd90 Mon Sep 17 00:00:00 2001 From: Kristian Lein-Mathisen Date: Mon, 14 Jun 2021 01:17:20 +0200 Subject: [PATCH 3/3] Fix for memory corruption There seems to be a problem with foreign-primitive related allocations. If the garbage collector runs right after (make-oid) and friends, the returned pointer is no longer valid. ~/p/chicken-git (master)> cat stress.scm (import git) (define repo (create-repository "tmp" #t)) (let loop () (create-blob repo #${4142}) (loop)) ~/p/chicken-git (master)> valgrind csi -s stress.scm ... ==56676== Invalid read of size 1 ==56676== at 0x5479264: git_oid_is_zero (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x5474509: git_odb_read (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x547154A: git_object_lookup_prefix (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x53DF95E: f_12197 (in /usr/lib/chicken/11/git.libgit2.so) ==56676== by 0x48F4FF0: ??? (in /usr/lib/libchicken.so.11) ==56676== by 0x491900A: ??? (in /usr/lib/libchicken.so.11) ==56676== by 0x49267B4: ??? (in /usr/lib/libchicken.so.11) ==56676== by 0x4B012B7: CHICKEN_run (in /usr/lib/libchicken.so.11) ==56676== by 0x4B02813: CHICKEN_main (in /usr/lib/libchicken.so.11) ==56676== by 0x4D7BB24: (below main) (in /usr/lib/libc-2.33.so) ==56676== Address 0x1ffef00514 is on thread 1's stack ==56676== 1046996 bytes below stack pointer ==56676== ==56676== Invalid read of size 1 ==56676== at 0x4843C77: bcmp (vg_replace_strmem.c:1111) ==56676== by 0x5472D2F: ??? (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x547456F: git_odb_read (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x547154A: git_object_lookup_prefix (in /usr/lib/libgit2.so.1.1.0) ==56676== by 0x53DF95E: f_12197 (in /usr/lib/chicken/11/git.libgit2.so) ==56676== by 0x48F4FF0: ??? (in /usr/lib/libchicken.so.11) This patch tries to address that. --- libgit2.scm | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/libgit2.scm b/libgit2.scm index f4e1518..06d3abe 100644 --- a/libgit2.scm +++ b/libgit2.scm @@ -17,6 +17,7 @@ (strict-types)) (import (scheme) + (only srfi-4 make-u8vector u8vector->blob/shared) (chicken base) (chicken bitwise) (chicken blob) @@ -196,8 +197,13 @@ (c-string email signature-email) ((struct time) when signature-time)) -(define-foreign-record-type (oid git_oid) - (unsigned-char (id (foreign-value GIT_OID_RAWSZ int)) oid-id)) +;; allow using both blobs and pointer as foreign oid argument types. +;; this lets us allocate space for an oid with make-blob. +;; unfortunately, it breaks the foreigners conventions of always using +;; c-pointers, so it can make things more confusing. revspec and buf +;; also follow this (mis)convention. +(define-foreign-type oid (c-pointer (struct "git_oid")) + (lambda (b) (if (blob? b) (location b) b))) (define-foreign-record-type (index-time git_index_time) (time-t seconds index-time-seconds) @@ -264,7 +270,8 @@ (define-foreign-type push (c-pointer "git_push")) (define-foreign-type note (c-pointer "git_note")) (define-foreign-type reference (c-pointer "git_reference")) -(define-foreign-type refspec (c-pointer "git_refspec")) +(define-foreign-type refspec (c-pointer "git_refspec") + (lambda (x) (if (blob? x) (location x) x))) ;; see make-revspec (define-foreign-type remote (c-pointer "git_remote")) (define-foreign-type remote-callbacks (c-pointer "git_remote_callbacks")) (define-foreign-type remote-head (c-pointer "git_remote_head")) @@ -280,12 +287,15 @@ ;;; buffer.h ;;; -(define-foreign-record-type (buf git_buf) - (c-string ptr buf-pointer buf-pointer-set!) - (size_t asize buf-asize buf-asize-set!) - (size_t size buf-size buf-size-set!)) +(define-foreign-type buf (c-pointer "git_buf") + (lambda (x) (if (blob? x) (location x) x))) -(define make-buf (foreign-primitive buf () "git_buf buf = {0}; C_return(&buf);")) +;; the buf-pointer that would have been generated by foreigners +;; doesn't respect our foreign buf type above as it's used in syntax. +(define buf-pointer (foreign-lambda* c-string ((buf buf)) "C_return(buf->ptr);")) + +;; note that buf must be 0-initialized (unlike oid and revspec) +(define (make-buf) (u8vector->blob/shared (make-u8vector (foreign-value "sizeof(git_buf)" int) 0))) (define buf-free (foreign-lambda void git_buf_free buf)) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -891,7 +901,7 @@ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; oid.h -(define make-oid (foreign-primitive oid () "git_oid oid; C_return(&oid);")) +(define (make-oid) (make-blob (foreign-value "sizeof(git_oid)" int))) (define oid-fromstr (foreign-lambda/allocate oid git_oid_fromstr nonnull-c-string)) (define oid-shorten-add (foreign-lambda/retval git_oid_shorten_add oid-shorten nonnull-c-string)) (define oid-cmp (foreign-lambda int git_oid_cmp oid oid)) @@ -1151,12 +1161,15 @@ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;; revparse.h -(define-foreign-record-type (revspec git_revspec) - (object from revspec-from) - (object to revspec-to) - (unsigned-int flags revspec-flags)) +;; as with oid and buf, we can't use foreigners do generate getters +;; for the revspec struct. +(define-foreign-type revspec (c-pointer "git_revspec") + (lambda (b) (if (blob? b) (location b) b))) + +(define revspec-from (foreign-lambda* object ((revspec r)) "C_return(r->from);")) +(define revspec-to (foreign-lambda* object ((revspec r)) "C_return(r->to);")) -(define make-revspec (foreign-primitive revspec () "git_revspec rev; C_return(&rev);")) +(define (make-revspec) (make-blob (foreign-value "sizeof(git_revspec)" int))) (define revparse-single (foreign-lambda/allocate object git_revparse_single repository nonnull-c-string)) (define revparse (foreign-lambda/allocate revspec git_revparse repository nonnull-c-string)) -- 2.31.1