Bug 8627 - Crash bug in dns_create_probe when dns_create_update fails
Summary: Crash bug in dns_create_probe when dns_create_update fails
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-22 10:29 UTC by Kai Blin
Modified: 2012-05-31 19:20 UTC (History)
2 users (show)

See Also:


Attachments
Patch for the crash bug for 3.6.1 (1.11 KB, patch)
2011-11-22 10:29 UTC, Kai Blin
no flags Details
Raw patch for 3.6.x. (482 bytes, patch)
2011-11-22 23:39 UTC, Jeremy Allison
no flags Details
Improved raw patch for 3.6.x (2.89 KB, patch)
2011-11-23 21:41 UTC, Jeremy Allison
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kai Blin 2011-11-22 10:29:56 UTC
Created attachment 7134 [details]
Patch for the crash bug for 3.6.1

When dns_create_update() fails, it does not initialize *req, and then TALLOC_FREE(req) in the error handling crashes if we're lucky.
Comment 1 Harry Mason 2011-11-22 11:20:04 UTC
Running net ads join where the domain name contained the '_' character (which violates RFC1034), it aborted with "Bad talloc magic value - unknown value".

Applied this patch to 3.6.0 and it resolved the problem. The DNS update fails as expected.
Comment 2 Jeremy Allison 2011-11-22 23:39:34 UTC
Created attachment 7138 [details]
Raw patch for 3.6.x.

I think this is an easier fix. We shouldn't have uninitialized variables on the stack here. Can you confirm it fixes the problem then we'll get it into 3.6.2 ?

Thanks !

Jeremy.
Comment 3 Harry Mason 2011-11-23 07:50:16 UTC
In my opinion the original patch is more consistent with how memory allocation is handled by other functions in this file, e.g. dns_create_update_request, dns_create_tsig_record, dns_unmarshall_tkey_record, etc. Kai and I discussed this alternative on IRC initially.

Arguably initializing these stack variables might aid debugging in future; in that case it ought to be applied to the entire file, as well as the change in the original patch.
Comment 4 Jeremy Allison 2011-11-23 21:41:34 UTC
Created attachment 7139 [details]
Improved raw patch for 3.6.x

You're right - this needs to be done on all possible error paths for functions in this file.

I prefer this fix (and it's similar to what has been added in master).

Jeremy.
Comment 5 Stefan Metzmacher 2012-05-30 11:29:59 UTC
Comment on attachment 7139 [details]
Improved raw patch for 3.6.x

Looks good
Comment 6 Stefan Metzmacher 2012-05-30 11:30:21 UTC
Karolin, please pick for the release
Comment 7 Karolin Seeger 2012-05-31 19:20:57 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!