[gelf] Replace '/' with '_' in keys when converting from msgpack.

[lbogdan]

Allows logs form Kubernetes pods having labels or annotations containing '/' in the name, which is not a valid GELF field name.

Also changes the log level to error and adds the invalid char position to "invalid key char" errors.

Fixes #1070 , #1291 .

[nigels-com]

Looks like it does as described.

Seems like there are potential leaks in the early-outs (out of memory situation).
Perhaps a goto cleanup block that centralises the cleanup of temporary copies?

[lbogdan]

Hey @nigels-com , thanks for your review and suggestion! I was aware of the memory leaks, but I didn't have any obvious fix for it, other than just freeing the copies in every branch, which would get quite verbose (on the other hand, I agree that's not really a reason to ignore memory leaks 🙂 ). I'll look for a goto pattern in the codebase and update the PR.

P.S.: I have used this patch in production for a week now, without any issues (the leak didn't trigger, as there were no further encoding errors).

[nigels-com]

In C++ this tends to be less of an issue, of course.

The sort of pattern I suggest is something like:

  void * a = NULL;
  void * b = NULL;
  void * c = NULL;
  void * result = NULL;

  a = malloc(len+1);
  ...
  {
    if (!b) goto cleanup;  /* Uh oh! Exit gracefully! */
    ...
  }
  /* Normal path of execution gets here */
  result = ...  /* Not a, b or c! */

  /* Do the necessary cleanup, whether or not there was an error */

cleanup:
  free(a);
  free(b);
  free(c);
  return result;

It's one of the few remaining legitimate uses of goto but a good one, in my opinion.
Assuming free(NULL) is harmless.

[lbogdan]

Done. I also safe-guarded the free()s with

if (copy) {
  flb_free(copy);
}

, just in case.

[nigels-com]

For the sake of portability, I'd recommend the NULL check. I do see NULL checks elsewhere in the codebase:

    if (ctx->merge_log_key) {
        flb_free(ctx->merge_log_key);
    }

[lbogdan]

@nigels-com I'm not sure I get what you mean, didn't I do exactly that?

[knackaron]

I applied this against our 1.1.0 image to fix the the same issue, works as intended.

@lbogdan Are you able to implement the modifications that @edsiper requested so this can get merged for the next release?

[lbogdan]

Hey @knackaron , sorry for the radio silence, I've been busy with work. I'll try to work on this next week, but feel free to take over, if you want.

[lbogdan]

Squashed the commits, updated the commit subject, should be ready for merge.

[knackaron]

@lbogdan No worries. Thanks for kicking out an updated patch over the weekend.

[PortaltechGithub]

Is there anything blocking the pull request? The requested change by @edsiper seems to be incorporated in the new commit.

[l-lotz]

The changes look good to me

[l-lotz]

@edsiper can you confirm that your requested change is now included?

[lbogdan]

There's also #1365 , which is replacing more than just / (although that's the only k8s "offending" character), so I guess only one of the two should be merged.

[isaacegglestone]

@lbogdan @edsiper This is definitely needed for kubernetes any chance we can get the slash tr merged since it appears to be done and only @edspiper needs to approve the change i think so its mergeable?

[edsiper]

thanks everyone!