Mies suomesta fixes segfaults and logic errors #515

MiesSuomesta · 2019-12-19T05:42:28Z

While debugging found some issues: Segfaults and made some securing actions.

Fixes some segfaults & tweaks

Fixed tests to pass (one testcase updated)

Restored printbuff default size to be 32 bytes.

hawicz

The vast majority of this pull request is not something that I would merge, and I see only three potentially useful changes:

Additional of a test (thank you!)
In sprintbuf(), the use of the PRINTBUF_DEFAULT_SIZE define and sizeof() instead of hard-coded constants, though that needs to fix the off-by-one bug and drop use of "long".
Perhaps, the fix to the order of the arguments to calloc() in json_object_new()

hawicz · 2019-12-19T13:28:12Z

json_object.c

@@ -467,6 +477,19 @@ struct lh_table* json_object_get_object(const struct json_object *jso)
 	}
 }

+static char *stringdup(const char *s)


What's wrong with strdup() ?

hawicz · 2019-12-19T13:28:43Z

json_object.c

@@ -220,14 +220,16 @@ static void json_object_generic_delete(struct json_object* jso)
 	lh_table_delete(json_object_table, jso);
 #endif /* REFCOUNT_DEBUG */
 	printbuf_free(jso->_pb);
+	jso->_pb = NULL;


Setting local variables and memory about to be freed to NULL is useless, please don't do this.

hawicz · 2019-12-19T13:32:28Z

json_object.c

-		r = "null";
-	}
-	else if ((jso->_pb) || (jso->_pb = printbuf_new()))
+	/* Failure case set first */


I don't see the point of this change. As far as I can tell, the overall logic is the same, but you've introduced an additional level of indentation, making the code slightly more complicated. Under what condition does the original code fail?

hawicz · 2019-12-19T13:32:50Z

json_object.c

-					 "\\u00%c%c",
-					 json_hex_chars[c >> 4],
-					 json_hex_chars[c & 0xf]);
+							 "\\u00%c%c",


I don't see any functional change here.

hawicz · 2019-12-19T13:33:31Z

json_object.c

-	existing_entry = (opts & JSON_C_OBJECT_ADD_KEY_IS_NEW) ? NULL : 
-			      lh_table_lookup_entry_w_hash(jso->o.c_object,
-							   (const void *)key, hash);
+	existing_entry = (opts & JSON_C_OBJECT_ADD_KEY_IS_NEW) ?


I don't see any functional change here.

hawicz · 2019-12-19T14:30:55Z

printbuf.c

@@ -142,14 +177,29 @@ int sprintbuf(struct printbuf *p, const char *msg, ...)

 void printbuf_reset(struct printbuf *p)
 {
-  p->buf[0] = '\0';
-  p->bpos = 0;
+  if (p != NULL)


It's not valid to call this with a NULL. At most, this could be an assert(), but crashing is perfectly reasonable too.

hawicz · 2019-12-19T14:31:40Z

printbuf.c

 }

 void printbuf_free(struct printbuf *p)
 {
  if(p) {
-    free(p->buf);
+
+	if (p->buf != NULL)


Pointless check, p->buf may never be NULL.

hawicz · 2019-12-19T14:32:32Z

printbuf.c

+	if (p->buf != NULL)
+		free(p->buf);
+
+    p->buf = NULL;


As with your changes in json_object_generic_delete, setting these to NULL is pointless.

hawicz · 2019-12-19T14:33:36Z

test_printbuf.c

+	printf("%s: end test\n", __func__);
+}
+
+#if 0


If the code isn't used, remove it.

hawicz · 2019-12-19T14:36:23Z

test_printbuf.c

+
+int main(int argc, char **argv)
+{
+	int before_resize = 0;


This before_resize values doesn't seem to make any sense. Why would the behavior of test_sprintbuf() depend on code that is entirely local to test_printbuf_memappend()? Please remove this.

MiesSuomesta · 2019-12-19T15:30:19Z

Ystävällisin tervehdyksin ja kaikkea hyvää toivoen, Lauri Jakku

On Thu, Dec 19, 2019 at 4:46 PM Eric Haszlakiewicz ***@***.***> wrote: ***@***.**** requested changes on this pull request. The vast majority of this pull request is not something that I would merge, and I see only three potentially useful changes: - Additional of a test (thank you!) - In sprintbuf(), the use of the PRINTBUF_DEFAULT_SIZE define and sizeof() instead of hard-coded constants, though that needs to fix the off-by-one bug and drop use of "long". - Perhaps, the fix to the order of the arguments to calloc() in json_object_new() ------------------------------ In json_object.c <#515 (comment)>: > @@ -467,6 +477,19 @@ struct lh_table* json_object_get_object(const struct json_object *jso) } } +static char *stringdup(const char *s) What's wrong with strdup() ?

On my system, strdup() caused segfault, so there.

------------------------------ In json_object.c <#515 (comment)>: > @@ -220,14 +220,16 @@ static void json_object_generic_delete(struct json_object* jso) lh_table_delete(json_object_table, jso); #endif /* REFCOUNT_DEBUG */ printbuf_free(jso->_pb); + jso->_pb = NULL; Setting local variables and memory about to be freed to NULL is useless, please don't do this. ------------------------------ In json_object.c <#515 (comment)>: > @@ -325,24 +327,30 @@ const char* json_object_to_json_string_length(struct json_object *jso, int flags const char *r = NULL; size_t s = 0; - if (!jso) - { - s = 4; - r = "null"; - } - else if ((jso->_pb) || (jso->_pb = printbuf_new())) + /* Failure case set first */ I don't see the point of this change. As far as I can tell, the overall logic is the same, but you've introduced an additional level of indentation, making the code slightly more complicated. Under what condition does the original code fail?

No, it is needed, the execute order of ((jso->_pb) || (jso->_pb = printbuf_new())) is

------------------------------ In json_object.c <#515 (comment)>: > @@ -149,9 +149,9 @@ static int json_escape_str(struct printbuf *pb, const char *str, int len, int fl str + start_offset, pos - start_offset); snprintf(sbuf, sizeof(sbuf), - "\\u00%c%c", - json_hex_chars[c >> 4], - json_hex_chars[c & 0xf]); + "\\u00%c%c", I don't see any functional change here.

I agree. I sort it out.

------------------------------ In json_object.c <#515 (comment)>: > @@ -481,9 +504,9 @@ int json_object_object_add_ex(struct json_object* jso, // We lookup the entry and replace the value, rather than just deleting // and re-adding it, so the existing key remains valid. hash = lh_get_hash(jso->o.c_object, (const void *)key); - existing_entry = (opts & JSON_C_OBJECT_ADD_KEY_IS_NEW) ? NULL : - lh_table_lookup_entry_w_hash(jso->o.c_object, - (const void *)key, hash); + existing_entry = (opts & JSON_C_OBJECT_ADD_KEY_IS_NEW) ? I don't see any functional change here.

I agree. I sort it out.

------------------------------ ------------------------------ In json_object.c <#515 (comment)>: > @@ -492,17 +515,26 @@ int json_object_object_add_ex(struct json_object* jso, if (!existing_entry) { - const void *const k = (opts & JSON_C_OBJECT_KEY_IS_CONSTANT) ? - (const void *)key : strdup(key); + char *key_dup = stringdup(key); I don't see why you wrote your own version of strdup. Also, even if there is a reason, it doesn't make sense to allocate and copy memory just to free it right away.

The strdup() in ? -clause did segfault on me, and the constant modification afterwards is not legal -> make strdup before it -> works ok. I'll try if standard strdup works, but in the ? -clause it did segfault.

------------------------------

In json_object.c <#515 (comment)>: > return lh_table_insert_w_hash(jso->o.c_object, k, val, hash, opts); + } else { Do not put this in an else block. It's a pointless change that merely increases the complexity of the code. ------------------------------ In json_object.c <#515 (comment)>: > @@ -1491,7 +1523,15 @@ int json_object_deep_copy(struct json_object *src, struct json_object **dst, jso int rc; /* Check if arguments are sane ; *dst must not point to a non-NULL object */ - if (!src || !dst || *dst) { + if (!src) { This is a pointless change, you're not changing the overall logic at all, just adding more lines of code.

I agree. I sort it out.

------------------------------ ------------------------------ In linkhash.c <#515 (comment)>: > @@ -563,7 +563,9 @@ void lh_table_free(struct lh_table *t) t->free_fn(c); } free(t->table); + t->table = NULL; As with the change you made in json_object_generic_delete, writing to memory that will shortly be unused is pointless, do not do this.

I agree. I sort it out.

------------------------------ ------------------------------ In printbuf.c <#515 (comment)>: > p->bpos = 0; - if(!(p->buf = (char*)malloc(p->size))) { + p->buf = (char*)calloc(1, p->size); Using calloc here is needlessly inefficient. There's no point clearing memory that we're going to copy real data in, and where we have a way (p->bpos) to track how much is actually used. Please do not do this. Also, this can mask bugs in other places by allowing sloppy code to seem to work because there happens to be a 0 byte.

I agree. I sort it out.

------------------------------ In printbuf.c <#515 (comment)>: > int new_size; - if (p->size >= min_size) + if( + (p != NULL) && It's not valid to pass in a NULL here, and silently returning without doing anything is not an appropriate thing to do. At most, I would accept an explicit assert() to check these invariants, but simply crashing in those cases is fine too.

I agree. I sort it out. I make an assert -> easier determine afterward what is the problem.

------------------------------ ------------------------------ In printbuf.c <#515 (comment)>: > return 0; new_size = p->size * 2; - if (new_size < min_size + 8) - new_size = min_size + 8; + + if (new_size < (min_size + PRINTBUF_EXTEND_BY_BYTES_MIN)) This looks like you're adding some arbitrary extra length to the buffer to hack around bugs elsewhere. Please do not do this. Instead, if you have some code that ends up writing beyond the end of the allocated buffer, *that* needs to be fixed.

I hate magic numbers in whatever code, the def is 8 bytes, if you did not notice.

------------------------------ ------------------------------ In printbuf.c <#515 (comment)>: > #ifdef PRINTBUF_DEBUG MC_DEBUG("printbuf_memappend: realloc " "bpos=%d min_size=%d old_size=%d new_size=%d\n", p->bpos, min_size, p->size, new_size); #endif /* PRINTBUF_DEBUG */ - if(!(t = (char*)realloc(p->buf, new_size))) + + if (p != NULL) It's pointless to check for p != NULL here, we already know that's the case.

I agree. I sort it out.

------------------------------ ------------------------------ In printbuf.c <#515 (comment)>: > #ifdef PRINTBUF_DEBUG MC_DEBUG("printbuf_memappend: realloc " "bpos=%d min_size=%d old_size=%d new_size=%d\n", p->bpos, min_size, p->size, new_size); #endif /* PRINTBUF_DEBUG */ - if(!(t = (char*)realloc(p->buf, new_size))) + + if (p != NULL) + { + t = (char*)calloc(1, new_size); Do not implement realloc yourself.

the realloc did segfault on me, but I agree. I sort it out.

------------------------------ ------------------------------ In printbuf.c <#515 (comment)>: > return 0; } int printbuf_memappend(struct printbuf *p, const char *buf, int size) { + + if ( (p->size > 0) && (p->buf == NULL)) { This makes no sense. If p->size>0 then buf MUST be non-NULL. At most this could be an assert() to validate that invariant, but it's also perfectly reasonable for this code to crash when things are corrupt like that, so please don't include this code.

There is a case there the buf was null, but size was not. I dont recall what was it. I agree. I sort it out, see if it is needed.

------------------------------

------------------------------ In printbuf.c <#515 (comment)>: > if (p->size <= p->bpos + size + 1) { - if (printbuf_extend(p, p->bpos + size + 1) < 0) - return -1; + if (printbuf_extend(p, p->bpos + size + 1) < 0) + return -2; Another useless change, this "-2" value isn't used anywhere.