search

quixdb / squash

Compression abstraction library and utilities
https://quixdb.github.io/squash/
MIT License
406 stars 53 forks source link

Change sizes to malloc() to size_t #160

Closed JakeSmarter closed 9 years ago

JakeSmarter commented 9 years ago

Regardless of the target platform, all size arguments to malloc() should be of type size_t. This helps to ensure the proper register word size on any given architecture.

nemequ commented 9 years ago

Obviously not getting through CI is a problem (the AppVeyor ones aren't because of this; hopefully f789c5a4 fixes it), that would need to be fixed before merging.

My worry here is that vsnprintf returns a negative value on error. If we stuff that into a size_t we may end up attempting to allocate a massive buffer. So, for the the C99-compliant version we need to put the value in an int at least temporarily. Technically _vscnprintf can also return a negative value, though AFAICT that should only happen if format is NULL, which we've already declared to be undefined behavior.

If we don't just put the result of _vscnprintf/vsnprintf into a size_t right away, is there any advantage in creating a temporary size_t to store the value just to pass it to malloc instead of depending on an implicit conversion in the function call?

We didn't properly handle negative return values, just pushed e5502d8c for that.

nemequ commented 9 years ago

BTW, unrelated to this issue, but you might want to check out #158; it's the result of our discussion the other day on IRC.

JakeSmarter commented 9 years ago

Obviously not getting through CI is a problem (the AppVeyor ones aren't because of this; hopefully f789c5a fixes it), that would need to be fixed before merging.

I think the errors have been produced because of some missing include, although I may be wrong.

My worry here is that vsnprintf returns a negative value on error. If we stuff that into a size_t we may end up attempting to allocate a massive buffer. So, for the the C99-compliant version we need to put the value in an int at least temporarily. Technically _vscnprintf can also return a negative value, though AFAICT that should only happen if format is NULL, which we've already declared to be undefined behavior.

If we don't just put the result of _vscnprintf/vsnprintf into a size_t right away, is there any advantage in creating a temporary size_t to store the value just to pass it to malloc instead of depending on an implicit conversion in the function call?

No, you're right. I have falsely assumed size_t to be signed (I don't know why I did it). The code is good as is. However, the compiler would still have produced a signed comparison because the 0 would have promoted the comparison to a signed comparison. size < 0u would have kept the comparison unsigned.

We didn't properly handle negative return values, just pushed e5502d8 for that.

From my personal point of view, the code looks good now (I am just trying to give some free advice, that's all). :-) Although I do not understand, why you bother with a stack allocated buffer in the first place, the code should work as expected on Windows now too. Anyway, I am glad I could help and meet a mind open to self improvement.

nemequ commented 9 years ago

Although I do not understand, why you bother with a stack allocated buffer in the first place

It's just an optimization. 512 bytes is enough for most uses, and allocating a 512 byte buffer on the stack is cheap. Doing so lets us avoid processing all the data a second time in the common case.

Anyways, thanks for the PR. It's very nice that someone is paying attention to details like this.