New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mies suomesta fixes segfaults and logic errors #515
Conversation
Fixes some segfaults & tweaks
Fixed tests to pass (one testcase updated)
Restored printbuff default size to be 32 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
@@ -467,6 +477,19 @@ struct lh_table* json_object_get_object(const struct json_object *jso) | |||
} | |||
} | |||
|
|||
static char *stringdup(const char *s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with strdup() ?
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting local variables and memory about to be freed to NULL is useless, please don't do this.
r = "null"; | ||
} | ||
else if ((jso->_pb) || (jso->_pb = printbuf_new())) | ||
/* Failure case set first */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
"\\u00%c%c", | ||
json_hex_chars[c >> 4], | ||
json_hex_chars[c & 0xf]); | ||
"\\u00%c%c", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any functional change here.
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) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any functional change here.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not valid to call this with a NULL. At most, this could be an assert(), but crashing is perfectly reasonable too.
} | ||
|
||
void printbuf_free(struct printbuf *p) | ||
{ | ||
if(p) { | ||
free(p->buf); | ||
|
||
if (p->buf != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointless check, p->buf may never be NULL.
if (p->buf != NULL) | ||
free(p->buf); | ||
|
||
p->buf = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with your changes in json_object_generic_delete, setting these to NULL is pointless.
printf("%s: end test\n", __func__); | ||
} | ||
|
||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code isn't used, remove it.
|
||
int main(int argc, char **argv) | ||
{ | ||
int before_resize = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
I agree. I sort it out and recheck this.
------------------------------
------------------------------
In printbuf.c
<#515 (comment)>:
> @@ -116,20 +151,20 @@ int sprintbuf(struct printbuf *p, const char *msg, ...)
{
va_list ap;
char *t;
- int size;
- char buf[128];
+ long int size;
vsprintf and vasprintf are defined to return int, not long int. Storing
the return value in a long isn't going to be useful.
I agree. I sort it out and recheck this.
------------------------------
------------------------------
In printbuf.c
<#515 (comment)>:
> @@ -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.
I agree. I sort it out and recheck this.
------------------------------
------------------------------
In printbuf.c
<#515 (comment)>:
> }
void printbuf_free(struct printbuf *p)
{
if(p) {
- free(p->buf);
+
+ if (p->buf != NULL)
Pointless check, p->buf may never be NULL.
I agree. I sort it out and recheck this.
------------------------------
------------------------------
In printbuf.c
<#515 (comment)>:
> }
void printbuf_free(struct printbuf *p)
{
if(p) {
- free(p->buf);
+
+ 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.
I agree. I sort it out and recheck this.
------------------------------
In test_printbuf.c
<#515 (comment)>:
> + printf("Append to just after resize: %d, [%s]\n", printbuf_length(pb), pb->buf);
+
+ free(data);
+ printbuf_free(pb);
+
+#define SA_TEST_STR "XXXXXXXXXXXXXXXX"
+ pb = printbuf_new();
+ printbuf_strappend(pb, SA_TEST_STR);
+ printf("Buffer size after printbuf_strappend(): %d, [%s]\n", printbuf_length(pb), pb->buf);
+ printbuf_free(pb);
+#undef SA_TEST_STR
+
+ printf("%s: end test\n", __func__);
+}
+
+#if 0
If the code isn't used, remove it.
I agree. I sort it out.
------------------------------
In test_printbuf.c
<#515 (comment)>:
> + printf("%d, [%s]\n", printbuf_length(pb), pb->buf);
+
+ sprintbuf(pb, "%d", INT_MIN);
+ printf("%d, [%s]\n", printbuf_length(pb), pb->buf);
+
+ sprintbuf(pb, "%s", "%s");
+ printf("%d, [%s]\n", printbuf_length(pb), pb->buf);
+
+ printbuf_free(pb);
+ printf("%s: end test\n", __func__);
+}
+#endif
+
+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.
I agree. I sort it out and recheck this.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#515?email_source=notifications&email_token=AH3M2SFGZNBVEKGB7IOEJB3QZOCLTA5CNFSM4J44BC3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPZAUQI#pullrequestreview-334629441>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH3M2SB53FADG5QMXWB2MGLQZOCLTANCNFSM4J44BC3A>
.
|
Seems that this was some other problem with the system, rebooted PC and recompiled 0.13.99 master.. no hick ups at all. |
While debugging found some issues: Segfaults and made some securing actions.