Bug 10002 - make the logging header customizable
make the logging header customizable
Status: ASSIGNED
Product: Samba 4.0
Classification: Unclassified
Component: Winbind
4.0.9
All All
: P5 normal
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-08 11:35 UTC by ravindra
Modified: 2014-05-08 23:11 UTC (History)
3 users (show)

See Also:


Attachments
Proposed patch (11.14 KB, patch)
2013-07-10 06:44 UTC, Matthieu Patou
no flags Details
The patch I think we need. (14.54 KB, patch)
2013-07-12 21:19 UTC, Jeremy Allison
no flags Details
Tested fix :-) (14.55 KB, patch)
2013-07-12 22:44 UTC, Jeremy Allison
mat: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ravindra 2013-07-08 11:35:56 UTC
Hi,

We have an use case where we trap winbindd logs from stdout. The trapped debug messages would be more useful in debugging and understanding if we prefix PID and time stamp etc to stdout log messages.

In that respect can we consider what Matthieu proposed in the following mail thread?

https://lists.samba.org/archive/samba-technical/2013-May/092858.html
Comment 1 Matthieu Patou 2013-07-10 06:44:19 UTC
Created attachment 9034 [details]
Proposed patch

Version of the patch for 4.0
Comment 2 Matthieu Patou 2013-07-10 06:45:57 UTC
Comment on attachment 9034 [details]
Proposed patch

Jeremy you promised to do the review. Can you do it please !
Comment 3 Jeremy Allison 2013-07-11 18:48:14 UTC
Unfortunately this patch has a bunch of bugs and off-by-one errors in length calculations which could overrun the string or leave it non-null terminated :-(.

It's easier to fix myself, so that's what I'm doing..

New patch will be uploaded shortly.

Jeremy.
Comment 4 Jeremy Allison 2013-07-11 20:11:20 UTC
This will be done by tomorrow (more changes than expected needed).

It is essentially a rewrite of the patch, so will need re-review by Matthieu to make sure it does what he wants before I'd be happy to push.

Also, we need an update of the smb.conf man page to go with this change.

Jeremy.
Comment 5 Jeremy Allison 2013-07-11 20:19:27 UTC
This line in the patch:

+               settings.debug_header_template = talloc_strdup(talloc_tos(),
+                                                              lp_debug_header_template());


will leave the global settings debug_header_template pointer with a pointer that will go out of scope ! This needs more fixing...
Comment 6 Matthieu Patou 2013-07-11 22:53:49 UTC
(In reply to comment #5)
> This line in the patch:
> 
> +               settings.debug_header_template = talloc_strdup(talloc_tos(),
> +                                                             
> lp_debug_header_template());
> 
> 
> will leave the global settings debug_header_template pointer with a pointer
> that will go out of scope ! This needs more fixing...

Interesting, should we do something like for the set_logfile where we actually talloc_dup the string with the NULL context ? 

ie something like that 
debug_set_logfile(lp_logfile(talloc_tos()));

Or attach a talloc context to the settings structure and everytime we reopen the logs we free the previous talloc context ?
Comment 7 Matthieu Patou 2013-07-11 22:59:41 UTC
(In reply to comment #3)
> Unfortunately this patch has a bunch of bugs and off-by-one errors in length
> calculations which could overrun the string or leave it non-null terminated
> :-(.
> 
Ok can you point them to me so that I don't do it next time I tried to be careful with the overrun by using snprintf I also tried with a very large template (> 200chars) and did see it being trunctated and also valgrind didn't complained.

Of course it didn't mean that it's failproof but still I'm willing to understand where are the mistakes.
> It's easier to fix myself, so that's what I'm doing..
> 
> New patch will be uploaded shortly.
> 
Ok great thanks for the rewrite.
Comment 8 Jeremy Allison 2013-07-12 17:39:08 UTC
Unfortunately snprintf doesn't null terminate (I know, that's why you used the len - 1 idiom).

The bug in dbgexpandtemplateparams() is here:

You have:

+               if (i >= size)
+                       return false;

to ensure i is one less than the avilable space, but then do:

+                               if (*chr) {
+                                       chr++;
+                               }
+                               dest[i] = '%';
+                               i++;
+                               dest[i] = '%';
+                               i++;

oops. You just incremented i twice. You only checked for one byte of space :-(.
Comment 9 Matthieu Patou 2013-07-12 18:15:32 UTC
(In reply to comment #8)
> Unfortunately snprintf doesn't null terminate (I know, that's why you used the
> len - 1 idiom).
> 
Really ?

Man pages says the opposite and this

#include <stdio.h> 

int main()
{
	char buf[20];
	char txt[] = "A very long string more than 20 chars actually, really I mean it";
	snprintf(buf, sizeof(buf), "len = %d %s", sizeof(txt) - 1, txt);
	fprintf(stderr, "buf = %s\n", buf);
	return 0;
}

Clearly show the opposite.
> The bug in dbgexpandtemplateparams() is here:
> 
> You have:
> 
> +               if (i >= size)
> +                       return false;
> 
> to ensure i is one less than the avilable space, but then do:
> 
> +                               if (*chr) {
> +                                       chr++;
> +                               }
> +                               dest[i] = '%';
> +                               i++;
> +                               dest[i] = '%';
> +                               i++;
> 
> oops. You just incremented i twice. You only checked for one byte of space :-(.

Oh right !
Comment 10 Jeremy Allison 2013-07-12 18:29:05 UTC
I don't think it guarantees null termination. At least it's really unclear to me. It's also unclear to a lot of other people :-).


http://www.unix.com/programming/65352-does-snprintf-guarantee-null-termination.html

Anyway, nearly finished with the rewrite.
Comment 11 Jeremy Allison 2013-07-12 18:33:17 UTC
Actually, this:

http://stackoverflow.com/questions/7706936/is-snprintf-always-null-terminating

says it is. So I can remove some of my code changes :-).
Comment 12 Volker Lendecke 2013-07-12 19:17:15 UTC
My reference, susv4, says:

    The snprintf() function shall be equivalent to sprintf(), with the addition
    of the n argument which states the size of the buffer referred to by s. If
    n is zero, nothing shall be written and s may be a null pointer. Otherwise,
    output bytes beyond the n-1st shall be discarded instead of being written
    to the array, and a null byte is written at the end of the bytes actually
    written into the array.

That is pretty clear language to me
Comment 13 Jeremy Allison 2013-07-12 21:19:28 UTC
Created attachment 9044 [details]
The patch I think we need.

Compiles but not yet tested. I'll work on that now. Thought you might want to look at the initial rewrite.

Jeremy.
Comment 14 Jeremy Allison 2013-07-12 22:44:41 UTC
Created attachment 9045 [details]
Tested fix :-)

Ok, this one works :-). Mat - please take a look and let me know what you think.

We'll need a docs fix also before this can be merged.

Jeremy.
Comment 15 Matthieu Patou 2013-07-15 07:30:06 UTC
Looks good, but let me recheck it tomorrow.
Comment 16 Jeremy Allison 2013-08-01 00:09:32 UTC
Ping ? I'd like to get this finished if it's going to go in :-).

Jeremy.
Comment 17 Matthieu Patou 2013-08-07 06:19:59 UTC
glou glou glou
I'm drowning.
Comment 18 Jeremy Allison 2013-08-19 23:47:13 UTC
Ping. If you +1 this I'll push it :-).

Jeremy.
Comment 19 Matthieu Patou 2013-09-19 19:20:41 UTC
Karolin, can you pick this fix for next 4.0.x release and also for 4.1.0 ?

Also I'll add a backport to 3.x.
Comment 20 Matthieu Patou 2014-05-02 06:16:00 UTC
Jeremy was it pushed upstream ? 
I didn't managed to find it.
Comment 21 ravindra 2014-05-06 17:53:08 UTC
This patch is really helpful in multiple winbindd child cases. Can we have the patch pushed to 4.X ?
Comment 22 Jeremy Allison 2014-05-06 17:57:27 UTC
I think I missed your +1 on the patch so it didn't get pushed upstream.

Let me do that first and then we'll get Karolin to add to 4.1.x and/or 4.0.x.

Jeremy.
Comment 23 Jeremy Allison 2014-05-06 18:14:15 UTC
Argh. Patch has now bit-rotted and doesn't apply any more to master :-(.

Matthieu, do you want to fix it up for current master ? If you want me to do it it's going to take a while..

Jeremy.