adds a stub for realloc #1035

gitoleg · 2020-01-22T18:41:53Z

We add a stub for realloc - surprisingly many allocations are done by passing a null pointer to realloc, and an analysis that is based on a tracing of allocation/freeing memory won't notice such call.

We also dropped unresolved_call linking from the promiscuous mode as it was interfering with our stack handling routines (it was preventing them from being triggered).

Also, we add a new behavior of the free call processing in memcheck-malloc: now, if the pointer being freed in unknown, i.e. there wasn't any memory allocation for it in the current primus machine, then we allocate one-byte length region and then release it. The justification is simple: by doing so, we are able to find more bugs like use-after-free or double-free, e.g. in some library, where any path we take won't necessarily contain the chain of calls like malloc->free->memory-access/free because such path exists only in the user code but not in the library itself.
ivg: however so far this feature produced only false positives, probably due to some unwanted aliasing between the pointer and pointee. Will keep in master for some time and see if it really a good idea.

We add a stub for realloc - surprisingly many allocations are done by passing a null pointer to realloc, and an analysis that is based on a tracing of allocation/freeing memory won't notice such call. Also, this PR fixes a minor bug in the method name in: there is no signal enter-jmp, so we can't depend on it. Also, we add a new behavior of the `free` call processing in memcheck-malloc: now, if the pointer being freed in unknown, i.e. there wasn't any memory allocation for it in the current primus machine, then we allocate one-byte length region and then release it. The justification is simple: by doing so, we are able to find more bugs like use-after-free or double-free, e.g. in some library, where any path we take won't necessarily contain the chain of calls like `malloc->free->memory-access/free` because such path exists only in the user code but not in the library itself.

ivg

Program semantics shall be defined by the state of the program and program itself. No magic dictionaries, please.

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

…pdates

ivg

In addition to the inline comments, please add documentation in the simple-memory-allocator.lisp of the chunk format.

ivg · 2020-01-30T17:02:03Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+
+(defun word-size () (/ (word-width) 8))
+
+(defun encode-memory-length(ptr len)


lisp style issue, please add a space between the function name and the list of arguments.

ivg · 2020-01-30T17:02:28Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+      (memory-allocate ptr len *malloc-initial-value*)
+    (memory-allocate ptr len)))
+
+(defun word-size () (/ (word-width) 8))


adding this function to the public interface will introduce confusion, between word-size and word-width so, please either remove it or mangle correspondingly.

ivg · 2020-01-30T17:04:49Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+(defun decode-memory-length(ptr)
+  (read-word ptr_t ptr))
+
+(defun decode-memory-length'(ptr)


I doubt that we need these two functions (again in the public interface).

mangle the function name, e.g., malloc/get-chunk-length

collapse them into one function.

ivg · 2020-01-30T17:13:50Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp


 (defun malloc (n)
  "allocates a memory region of size N"
  (declare (external "malloc"))
  (if (= n 0) *malloc-zero-sentinel*
    (if (malloc-will-reach-limit n) 0
-      (let ((n (+ n (* 2 *malloc-guard-edges*)))
+      (let ((width (word-size))


a) we always use the word width for the size in bits. And in general, unless specified otherwise, we prefer bits to bytes. In that case, you maximize the astonishment when you first define a function word-size that translates word width into size in bytes and the bind it to a variable that is named width. This code will break on the next update...

b) please choose variable names that bear some semantics, instead of using generic width, array, etc. In this particular case, I suggest header-size, e.g.,

(let ((header-size (word-width / 8)) ...)

ivg · 2020-01-30T17:29:42Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp


 (defun malloc (n)
  "allocates a memory region of size N"
  (declare (external "malloc"))
  (if (= n 0) *malloc-zero-sentinel*
    (if (malloc-will-reach-limit n) 0
-      (let ((n (+ n (* 2 *malloc-guard-edges*)))
+      (let ((width (word-size))
+            (full (+ n (* 2 *malloc-guard-edges*) width))


rename full to chunk-size

ivg · 2020-01-30T17:30:37Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+          (set brk (+ brk full))
+          (malloc/fill-edges ptr full)
+          (set ptr (+ ptr *malloc-guard-edges*))
+          (encode-memory-length ptr n)


rename encode/decode to get/put or load/store or read/write. You choice.

ivg · 2020-01-30T18:03:58Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+          (encode-memory-length ptr n)
+          (+ ptr width))))))
+
+(defun realloc (ptr new)


Please, rewrite this function to conform with POSIX. In particular,

when ptr is NULL it shall behave as malloc

when len is NULL it shall behave as free

when len is unchanged nothing should be done

when len is smaller the pointer shall not change, but see a not below.

only when len is greater than the old-len we may return a new pointer (and we don't have other choices with our current allocator).

if a new chunk is allocated by realloc the old chunk, pointed by ptr shall be deallocated.
The following code (untested) implements this semantics. Note, how (or (and ..) (and ..) ...) is used in place where you will be using pattern matching in OCaml. This is much easier to read than a bunch of nested if/then/elses.

(defun realloc (ptr len) (or (and (not ptr) (malloc ptr len)) (and (not len) (free ptr)) (let ((old-len (malloc/get-chunk-size ptr))) (if (<= old-len len) (prog (malloc/put-chunk-size ptr len) ptr) (free ptr) (let (new-ptr (malloc ptr len)) (if new-ptr (memcpy new-ptr ptr old-len) new-ptr))))))

Note, if realloc is used to reduce the size of a chunk (e.g., for shrinking a dynamic vector), we shall return the same pointer, but mark the region ptr+len,ptr+old-len as invalid. Therefore, we need to update the region-analysis (where malloc is hooked into it, and track calls to realloc which shrink the regions and try to mark them as freed). If it would be too hard, we can ignore it right now and think about it later.

This realloc is also buggy, by the way, the trick (or (and <cond> <body>) ...) doesn't work here, since <body> in our case could also evaluate to false so the control flow will fall down, i.e., both malloc and free could return false, in fact free always returns false, so we will fall after free to realloc. It won't break a lot, i.e., we will now hit the (<= old-len len) guard, and just write 0 to the length field. But still it is more a bug than a feature.

ivg · 2020-01-30T18:21:15Z

plugins/primus_test/lisp/memcheck-malloc.lisp

@@ -3,6 +3,9 @@
 (defmethod call (name ptr)
  (when (and ptr (= name 'free)
             (not (= ptr *malloc-zero-sentinel*)))
+    (when (and ptr


ptr is already non-NULL here, see the guard above. Also for when (not cond) there is ~~unless~~ until cond. Ooops, it should be, of course, unless, not until and not even unit (as it is documented) :) Could you please add unless in addition to (the now deprecated) until to the init.lisp? And, please, update the docs correspondingly, and mark until as deprecated.

So the code will be now much more readable:

(defmethod call (name ptr) (when (and ptr (= name 'free) (not (= ptr *malloc-zero-sentinel*))) (unless (memcheck-is-allocated 'malloc ptr) (memcheck-acquire 'malloc ptr 1)) (memcheck-release 'malloc ptr)))

Ok, I was really confused yesterday :) unless is not until. unless is a dual for when, where until is dual while. So here are the definitions that we should have,

(defmacro when (cnd body) "(when CND BODY) if CND evaluates to true, then BODY is evaluated and the value of the last expression in BODY becomes the value of the whole expression. Otherwise, if CND evaluates to false, nil is returned." (if cnd (prog body) ())) (defmacro unless (cnd body) "(unless CND BODY) if CND evaluates to false, then BODY is evaluated and the value of the last expression in BODY becomes the value of the whole expression. Otherwise, if CND evaluates to true, nil is returned." (if (not cnd) () body)) (defmacro until (c b) "(until COND BODY) if COND evaluates to true, then the whole expression evaluates to nil and BODY is not evaluated. Otherwise, if COND evaluates to false, then BODY is evaluated until COND evaluates to true and the value of the last evaluation of BODY becomes the value of the whole expression." (while (not c) b))

ivg · 2020-01-30T18:22:27Z

plugins/primus_test/lisp/memcheck.lisp

@@ -24,6 +24,12 @@
 (require incident)

 ;; Public Interface
+
+(defun memcheck-is-allocated (heap ptr)


I would name it memcheck-is/was-allocated or even better, memcheck-is-tracked to prevent future confusion.

ivg · 2020-02-03T16:06:51Z

plugins/primus_lisp/lisp/simple-memory-allocator.lisp

+          (+ ptr header-size))))))
+
+
+(defun realloc/update-chunk-size (ptr len)


rename it to realloc/shrink-chunk

ivg

LGTM, but I want #1047 to be merged before this PR, so that the new lisp codes could be type-checked. There is also quite a few intersections, so it will become clear after #1047 is merged.

…w-primus-lisp-updates

plugins/primus_test/lisp/memcheck-malloc.lisp

ivg requested changes Jan 24, 2020

View reviewed changes

plugins/primus_lisp/lisp/simple-memory-allocator.lisp Outdated Show resolved Hide resolved

gitoleg added 3 commits January 28, 2020 15:44

encoded length

bf31a28

Merge remote-tracking branch 'upstream/master' into few-primus-lisp-u…

73f9c13

…pdates

removed unused function

8e74a24

gitoleg added the ready for review label Jan 29, 2020

ivg requested changes Jan 30, 2020

View reviewed changes

ivg removed the ready for review label Jan 30, 2020

gitoleg added 2 commits January 31, 2020 15:15

rewrote realloc

2491fcd

refactored a little

90d5d18

gitoleg requested a review from ivg January 31, 2020 21:33

ivg requested changes Feb 3, 2020

View reviewed changes

renaming

e393461

gitoleg requested a review from ivg February 5, 2020 16:38

gitoleg added the ready for review label Feb 6, 2020

ivg requested changes Feb 7, 2020

View reviewed changes

gitoleg force-pushed the few-primus-lisp-updates branch 2 times, most recently from 0ab0b9b to c7a82d5 Compare February 12, 2020 18:28

merge with master

d8080fe

gitoleg force-pushed the few-primus-lisp-updates branch from c7a82d5 to d8080fe Compare February 12, 2020 18:30

gitoleg added 3 commits February 13, 2020 09:54

removed unresolved names handling from promiscuous

a1ced40

merge with master

c272546

Merge remote-tracking branch 'origin/few-primus-lisp-updates' into fe…

659cb45

…w-primus-lisp-updates

ivg reviewed Feb 26, 2020

View reviewed changes

plugins/primus_test/lisp/memcheck-malloc.lisp Show resolved Hide resolved

ivg changed the title ~~few primus-lisp updates~~ adds a stub for realloc Feb 26, 2020

ivg approved these changes Feb 26, 2020

View reviewed changes

ivg merged commit cd024cf into BinaryAnalysisPlatform:master Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds a stub for realloc #1035

adds a stub for realloc #1035

gitoleg commented Jan 22, 2020 •

edited by ivg

Loading

ivg left a comment

ivg left a comment

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 30, 2020

ivg Jan 31, 2020

ivg Jan 30, 2020

ivg Jan 31, 2020

ivg Jan 30, 2020

ivg Feb 3, 2020

ivg left a comment


		(defun word-size () (/ (word-width) 8))

		(defun encode-memory-length(ptr len)

		(+ ptr header-size))))))


		(defun realloc/update-chunk-size (ptr len)

adds a stub for realloc #1035

adds a stub for realloc #1035

Conversation

gitoleg commented Jan 22, 2020 • edited by ivg Loading

ivg left a comment

Choose a reason for hiding this comment

ivg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivg left a comment

Choose a reason for hiding this comment

gitoleg commented Jan 22, 2020 •

edited by ivg

Loading