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
Created attachment 9034 [details] Proposed patch Version of the patch for 4.0
Comment on attachment 9034 [details] Proposed patch Jeremy you promised to do the review. Can you do it please !
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.
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.
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...
(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 ?
(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.
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 :-(.
(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 !
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.
Actually, this: http://stackoverflow.com/questions/7706936/is-snprintf-always-null-terminating says it is. So I can remove some of my code changes :-).
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
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.
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.
Looks good, but let me recheck it tomorrow.
Ping ? I'd like to get this finished if it's going to go in :-). Jeremy.
glou glou glou I'm drowning.
Ping. If you +1 this I'll push it :-). Jeremy.
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.
Jeremy was it pushed upstream ? I didn't managed to find it.
This patch is really helpful in multiple winbindd child cases. Can we have the patch pushed to 4.X ?
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.
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.