On 07.09.2016 08:45, Sebastian Lackner wrote:
On 06.09.2016 15:26, Sebastian Lackner wrote:
On 06.09.2016 12:27, Jacek Caban wrote:
Hi Sebastian,
I took a deeper look at this. It seems that we have at least two more jsstr_alloc_buf callers to fix (jsstr_concat and JSGlobal_decodeURIComponent). All those seem to be mistakes caused by wrong choice of function signature, requiring not intuitive error handling. I'd prefer to change the root of those problems, making the function: jsstr_t *jsstr_alloc_buf(unsigned,WCHAR**) This should avoid similar mistakes in the future.
This will be much larger patch than you originally meant, so please let me know if you'd like to prepare a patch or should I take care of that.
Thanks, Jacek
Good catch. Yes, the current function signature makes it very easy to do things wrong. If you want me to prepare an updated patch I can do that, but I also don't mind if you want to prepare a patch yourself.
Best regards, Sebastian
I've just sent a patch, but feel free to ignore it if you have already been working on your own version.
Thanks.
By the way, there is another aspect which makes jsstr_alloc_buf very hard to use, which is that we have to know the exact buffer size in advance. As a result there are also places where it is done wrong, for example Date_toTimeString which passes the maximum required buffer size, but does not update ->length_flags afterwards. Are you fine with adding something like
jsstr_shrink_buf(jsstr_t *) or jsstr_shrink_buf(jsstr_t *, unsigned)
to fix this, or do we really want to fix the length calculation instead?
I'm not opposed to this function if really needed, but in cases where we can avoid it, I'd much rather do that. In case of Date_toTimeStringi avoiding it seems easy. There is constant and relatively small maximum buffer size and the code path is not performance critical, so I'd say that using a buffer on stack to prepare the result and then pass it to jsstr_alloc would be nicer IMHO.
Thanks, Jacek