Hi no problems it is not a bugfix at all, usually just better to copy a
const char instead of casting it in this sort of situation. Nothing else.
Best regards.
On 28 February 2016 at 22:58, Willy Tarreau <w...@1wt.eu> wrote:
> Hi David,
>
> Sorry for the delay, but each time I looked at it I started to scratch my
> head wondering about the real purpose of the change and/or the reason for
> the current code currently being that way.
>
> On Thu, Feb 18, 2016 at 09:58:49PM +0000, David CARLIER wrote:
> > HI all,
> >
> > There is a tiny patch proposal for the log component.
>
>
> From f1924d813f37cd6444d9251605fdd823efb7fe20 Mon Sep 17 00:00:00 2001
> From: David Carlier <devne...@gmail.com>
> Date: Thu, 18 Feb 2016 21:46:48 +0000
> Subject: [PATCH] CLEANUP: log
>
> The string to be escaped is not marked at const,
> and it would be safer to copy instead of casting.
>
> ---
> src/log.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/log.c b/src/log.c
> index 3e25bc7..94864e1 100644
> --- a/src/log.c
> +++ b/src/log.c
> @@ -831,13 +831,19 @@ char *lf_text_len(char *dst, const char *src, size_t
> len, size_t size, struct lo
> if (src && len) {
> if (node->options & LOG_OPT_ESC) {
> struct chunk chunk;
> - char *ret;
> + char *ret, *ssrc;
>
> - chunk_initlen(&chunk, (char *)src, 0, len);
> + ssrc = my_strndup(src, len);
> + if (ssrc == NULL)
> + return NULL;
> + chunk_initlen(&chunk, ssrc, 0, len);
> ret = escape_chunk(dst, dst + size, '\\',
> rfc5424_escape_map, &chunk);
> - if (ret == NULL || *ret != '\0')
> + if (ret == NULL || *ret != '\0') {
> + chunk_destroy(&chunk);
> return NULL;
> + }
> len = ret - dst;
> + chunk_destroy(&chunk);
> }
> else {
> if (++len > size)
>
> We cannot afford to call strndup() for each log element to be escaped, it
> adds too much overhead, it performs a malloc()! I'd suggest two approaches
> (I haven't looked at the calling places) :
> - either you simply use another chunk as a temporary holder for that
> part ;
> - or we check if it's acceptable to change the const char* for a char *
> in the argument and expect the caller to have a R/W buffer itself.
>
> Hmmm just thinking about it, it looks like we don't modify src anyway here,
> so it's even more overkill to strdup() something we're not going to change.
>
> Thus I'd rather go for something much cleaner : let's add a new function
> "chunk_initlen_const()" to chunk.h taking a const char* and a length, and
> performing the cast itself, as well as leaving the size to zero (so that
> we can't modify the contents). Then you just have to make use of it in the
> function above and the problem is gone. It's possible that other cases
> exist as well by the way.
>
> Best regards,
> Willy
>
>
