Two questions about the function of ensure and print

[liuyunbin]

1. In static unsigned char* ensure(printbuffer * const p, size_t needed), needed whether include '\0'? In cJSON.c, most case needed include '\0', but the line of 515, 1451, 1641, 1661, needed don't include '\0'.

2. In static unsigned char *print(const cJSON * const item, cJSON_bool format, const internal_hooks * const hooks), about line 1098 should add buffer->length = 256;, the line 1114 should be printed = (unsigned char*) hooks->reallocate(buffer->buffer, buffer->offset + 1);, in my opinion.

I'm sorry for my poor English.

[FSMaxB]

Thanks for your code review. I will have a closer look at all the calls to ensure and all length variables and fix what I can find.

Since some of these off by one errors could potentially have security implications, I will make a new release as fast as possible.

[FSMaxB]

Looking at your examples in detail, there are 2 noncritical bugs and one Off-By-One error that could lead to an out of bound write of one byte. The other lines of code are correct.

I will look through everything else again to be more confident that there is nothing more though.

1

This definitely looks like an off by one error:

cJSON/cJSON.c

Line 515 in 984dc85

output_pointer = ensure(output_buffer, (size_t)length);

This is correct, because \0 is not written to the output before later ensuring enough space for it:

cJSON/cJSON.c

Line 1451 in 984dc85

output_pointer = ensure(output_buffer, 1);

This again is correct, because \0 is not written to the output before print_string_ptr ensures enough space in the output:

cJSON/cJSON.c

Line 1641 in 984dc85

output_pointer = ensure(output_buffer, output_buffer->depth);

The next one is also correct, because no \0 is written to the output and then print_value is called, which will ensure that a \0 is written:

cJSON/cJSON.c

Line 1661 in 984dc85

output_pointer = ensure(output_buffer, length);

2

This is definitely a bug (not assigning the length to buffer->length. And it probably has quite some impact on performance:

cJSON/cJSON.c

Lines 1095 to 1102 in 984dc85

	/* create buffer */
	buffer->buffer = (unsigned char*) hooks->allocate(256);
	buffer->format = format;
	buffer->hooks = *hooks;
	if (buffer->buffer == NULL)
	{
	goto fail;
	}

Also a bug, this makes reallocating completely pointless:

cJSON/cJSON.c

Line 1114 in 984dc85

printed = (unsigned char*) hooks->reallocate(buffer->buffer, buffer->length);

[FSMaxB]

I didn't find anything else, except some places where ensure checked one more than was actually necessary. But I won't change them in a bug fix release.

[FSMaxB]

Fixed in Release 1.7.1

[liuyunbin]

You are right. And I think the line

cJSON/cJSON.c

Line 384 in 984dc85

if ((p->length > 0) && (p->offset >= p->length))

should be if (p->offset > p->length)

the line

cJSON/cJSON.c

Line 396 in 984dc85

needed += p->offset + 1;

should be needed += p->offset;

the line

cJSON/cJSON.c

Line 1127 in 984dc85

memcpy(printed, buffer->buffer, cjson_min(buffer->length, buffer->offset + 1));

should be memcpy(printed, buffer->buffer, buffer->offset + 1));

Because p->offset is the next index could be written, if (p->offset == p->length) is ok.

@FSMaxB

[FSMaxB]

Thanks for the further review.

You are right. And I think the line

cJSON/cJSON.c

Line 384 in 984dc85

if ((p->length > 0) && (p->offset >= p->length))

should be if (p->offset > p->length)

Well the > 0 is useless, you're right. I think it is from a time where p->length was an integer. But I cannot replace the >= with a > because the offset is equivalent to an array index and the length the size of the array. So the offset must never be length because p->buffer[p->offset] would then be an out of bounds access.

the line

cJSON/cJSON.c

Line 396 in 984dc85

needed += p->offset + 1;

should be needed += p->offset;

No, for the same reason. If offset is 5 for example, the buffer needs to be 6 bytes long. So everything correct here.

the line

cJSON/cJSON.c

Line 1127 in 984dc85

memcpy(printed, buffer->buffer, cjson_min(buffer->length, buffer->offset + 1));

should be memcpy(printed, buffer->buffer, buffer->offset + 1));

Because p->offset is the next index could be written, if (p->offset == p->length) is ok.

This cjson_min is just protecting from overreading the old buffer. But since buffer->offset + 1 should always be <= buffer->length, this cjson_min can probably removed. I'll look into that.