Hi Evan,
And thanks for getting back to me. I found another problem related to
memory corruption and I've been digging around. I have patches for that
too, but I don't know if they suffice. They sort of break existing
conventions of everything in libgit2 being foreigners and c-pointers.
I'm attaching my patches here in case you have time to look into this.
The attachments here include the fixes in the previous mail too, so please
ignore that one.
>From what I gather, there is no way to allocate memory and return a pointer
to it, safely, in CHICKEN.
Won't the garbage collector potentially overwrite whatever region was
allocated since it has no way of knowing it?
That's why I made (make-oid) and friends return chicken blobs instead. Let
me know if there's a better way!
K.
On Sun, Jun 13, 2021 at 11:19 PM Evan Hanson <ev...@foldling.org> wrote:
> Hi Kristian,
>
> On 2021-06-12 15:36, Kristian Lein-Mathisen wrote:
> > I think I may have come across a bug in the git egg.
>
> You're right! THanks for pointing this out, I guess you must be the
> first person to use that `frombuffer` procedure.
>
> Everything you've said looks correct to me, I'll try to publish a fix
> for this soon.
>
> Evan
>
From 602f415a5fe769b786efb2abd03a480661b6e0c4 Mon Sep 17 00:00:00 2001
From: Kristian Lein-Mathisen <kristianl...@gmail.com>
Date: Sun, 13 Jun 2021 22:56:18 +0200
Subject: [PATCH 1/3] Fix mismatched blob-create-frombuffer signature
Before this patch, the new test case would produce an error like this:
Error: bad argument count - received 2 but expected 3: #<procedure>
---
git.scm | 2 +-
libgit2.scm | 2 +-
tests/run.scm | 1 +
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/git.scm b/git.scm
index b33cd45..c8762d1 100644
--- a/git.scm
+++ b/git.scm
@@ -785,7 +785,7 @@
(git-blob-lookup
repo*
(cond ((chicken-blob? source)
- (git-blob-create-frombuffer repo* source))
+ (git-blob-create-frombuffer repo* source (number-of-bytes source)))
((string? source)
(if (regular-file? source)
(git-blob-create-fromdisk repo* source)
diff --git a/libgit2.scm b/libgit2.scm
index a1e095c..83334c3 100644
--- a/libgit2.scm
+++ b/libgit2.scm
@@ -295,7 +295,7 @@
(define blob-lookup-prefix (foreign-lambda/allocate blob* git_blob_lookup_prefix repository oid unsigned-int))
(define blob-create-fromdisk (foreign-lambda/allocate oid git_blob_create_fromdisk repository nonnull-c-string))
(define blob-create-fromworkdir (foreign-lambda/allocate oid git_blob_create_fromworkdir repository nonnull-c-string))
-(define blob-create-frombuffer (foreign-lambda/allocate oid git_blob_create_frombuffer repository nonnull-c-string unsigned-int))
+(define blob-create-frombuffer (foreign-lambda/allocate oid git_blob_create_frombuffer repository nonnull-scheme-pointer size_t))
(define blob-id (foreign-lambda/copy oid git_blob_id blob*))
(define blob-free (foreign-lambda void git_blob_free blob*))
(define blob-rawcontent (foreign-lambda c-pointer git_blob_rawcontent blob*))
diff --git a/tests/run.scm b/tests/run.scm
index b73e97f..7ede8da 100644
--- a/tests/run.scm
+++ b/tests/run.scm
@@ -393,6 +393,7 @@
(test-group "blob"
(test-error "blob with nonexistent sha" (blob repo (make-string 40 #\a)))
+ (test "create-blob from memory source" 4 (blob-length (create-blob repo #${41424344})))
(let ((t (commit-tree commit)))
(with-current-directory repo-path
(lambda ()
--
2.31.1
From 621f8a0178d2f66470ca978716b4a62f33f2dd90 Mon Sep 17 00:00:00 2001
From: Kristian Lein-Mathisen <kristianl...@gmail.com>
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
From 9f11e4929b357866bc6fba8972888071686a2c97 Mon Sep 17 00:00:00 2001
From: Kristian Lein-Mathisen <kristianl...@gmail.com>
Date: Sun, 13 Jun 2021 22:58:08 +0200
Subject: [PATCH 2/3] Fix blob function reference typo
I can't figure out where these are used, but this is certainly better.
---
libgit2.scm | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libgit2.scm b/libgit2.scm
index 83334c3..f4e1518 100644
--- a/libgit2.scm
+++ b/libgit2.scm
@@ -303,9 +303,9 @@
(define blob-is-binary (foreign-lambda bool git_blob_is_binary blob*))
(define blob*-lookup blob-lookup)
-(define blob*-create-frombuffer blob-create-fromdisk)
-(define blob*-create-fromdisk blob-create-fromworkdir)
-(define blob*-create-fromworkdir blob-create-frombuffer)
+(define blob*-create-frombuffer blob-create-frombuffer)
+(define blob*-create-fromdisk blob-create-fromdisk)
+(define blob*-create-fromworkdir blob-create-fromworkdir)
(define blob*-free blob-free)
(define blob*-id blob-id)
(define blob*-is-binary blob-is-binary)
--
2.31.1
