Re: [PATCH] Avoid allocating 0 bytes, was Re: [PATCH 4/4] git-compat-util.h: dietlibc-friendly x{malloc,realloc,calloc}

Hi,

On Mon, 26 Dec 2005, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

diff --git a/csum-file.c b/csum-file.c
index 5f9249a..2c0f097 100644
--- a/csum-file.c
+++ b/csum-file.c

@@ -121,6 +121,9 @@ int sha1write_compressed(struct sha1file
 	unsigned long maxsize;
 	void *out;
 
+	if (size == 0)
+		return 0;
+
 	memset(&stream, 0, sizeof(stream));
 	deflateInit(&stream, Z_DEFAULT_COMPRESSION);
 	maxsize = deflateBound(&stream, size);

I think this and the one in sha1_file.c::write_sha1_file() are
wrong; 0-size input would not result in 0-size output.  Have you
tested them by actually exercising the codepaths you touched?

No, I did not test them. At least not consciously (I did the whole work on 
Dec 24, when I was lying in bed, ill, trying to distract myself from being 
miserable).

The reason I did it this way: If zlib inflates a buffer of 0 bytes, it 
makes no sense to expect anything than 0 bytes to come out of it, right? 
Therefore, if zlib encounters a deflated buffer of 0 bytes, it should 
inflate it to 0 bytes. So it is a good idea in any case to write out 0 
bytes.

However, thinking of it again, it might break backwards compatibility. But 
I don't think so.

quoted

diff --git a/diffcore-pathspec.c b/diffcore-pathspec.c
index 68fe009..a12337a 100644
--- a/diffcore-pathspec.c
+++ b/diffcore-pathspec.c

@@ -48,10 +48,14 @@ void diffcore_pathspec(const char **path

diffcore-pathspec and diffcore-order can probably return without
touching diff_queued_diff if there is no work to be done.

To be honest: I did not completely understand what that code does. 
It seemed obvious, but I got a track record of breaking things. So I was 
cautious. I will not try to modify it myself without a good test case.

quoted

@@ -353,7 +353,8 @@ static void write_index_file(const char 
 {
 	struct sha1file *f;
 	struct object_entry **sorted_by_sha =
-		xcalloc(nr_objects, sizeof(struct object_entry *));
+		xcalloc(nr_objects ? nr_objects : 1,
+				sizeof(struct object_entry *));
 	struct object_entry **list = sorted_by_sha;
 	struct object_entry **last = sorted_by_sha + nr_objects;
 	unsigned int array[256];

This can be simplified by sorted_by_sha = list = last = NULL
when nr_objects == 0 and avoiding qsort; that is what you did in
pack-objects, I think.

If nr_objects == 0, we could write out a fixed index.

We could alternatively exit(1), because what sense does it make to have an 
index if the pack contains 0 objects? However, this has consequences: 
Every user of index-pack (does "fetch-pack --keep" call it?) has to be 
aware that empty packs are illegal.

Again, I did this so that the tool does not break. I'll try to come up 
with something better soon.

quoted

@@ -448,8 +449,10 @@ int main(int argc, char **argv)
 
 	open_pack_file();
 	parse_pack_header();
-	objects = xcalloc(nr_objects, sizeof(struct object_entry));
-	deltas = xcalloc(nr_objects, sizeof(struct delta_entry));
+	objects = xcalloc(nr_objects ? nr_objects : 1,
+			sizeof(struct object_entry));
+	deltas = xcalloc(nr_objects ? nr_objects : 1,
+			sizeof(struct delta_entry));
 	parse_pack_objects();
 	free(deltas);
 	write_index_file(index_name, sha1);

Likewise I suspect.  After all the special case is only when
reindexing an empty pack ;-).

Right.

My inclination is to do things in these steps:

 - apply cleanups that actually simplify the logic, while
   leaving the ones that you needed to do (size ?  size : 1)
   unmodified (BTW, must next_in/next_out point at non NULL when
   avail_in/avail_out are zero?).

I was cautious. Maybe at some point in future, zlib relies on that. But it 
is probably better to try not calling zlib at all in these cases.

 - change x*alloc like this, once the above is done:

        static inline void *xmalloc(size_t size)
        {
                void *ret = malloc(size);
        #ifdef MALLOC_CAN_RETURN_NULL_ON_0SIZE
                if (!ret && !size)
                        ret = malloc(size+1);
        #endif
                if (!ret)
                        die("Out of memory, malloc failed");
                return ret;
        }

Sounds easiest.

Ciao,
Dscho