[PATCH] s3-cli: do not segfault when a Cyrillic workgroup is used

David Disseldorp ddiss at suse.de
Mon Mar 21 12:14:54 MDT 2011


msrpc_gen() assumes that a zero terminator is always included in the
output length returned on success by convert_string_talloc(). This is
currently an incorrect assumption as under certain conditions (Cyrillic
input is one such case) an output string length of zero may be returned
with successful status.

This results in the decrement following conversion in msrpc_gen() to
wrap the length variable, prior to use in a memcpy().

This change protects msrpc_gen() for zero length returns. A more proper
fix would be to have a consistent API for convert_string_talloc().
---
 libcli/auth/msrpc_parse.c |    9 ++++++---
 source3/lib/charcnv.c     |    7 ++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/libcli/auth/msrpc_parse.c b/libcli/auth/msrpc_parse.c
index 7ac6fb5..b259581 100644
--- a/libcli/auth/msrpc_parse.c
+++ b/libcli/auth/msrpc_parse.c
@@ -75,7 +75,8 @@ bool msrpc_gen(TALLOC_CTX *mem_ctx,
 				return false;
 			}
 			pointers[i].length = n;
-			pointers[i].length -= 2;
+			if (pointers[i].length >= 2)
+				pointers[i].length -= 2;
 			data_size += pointers[i].length;
 			break;
 		case 'A':
@@ -89,7 +90,8 @@ bool msrpc_gen(TALLOC_CTX *mem_ctx,
 				return false;
 			}
 			pointers[i].length = n;
-			pointers[i].length -= 1;
+			if (pointers[i].length >= 1)
+				pointers[i].length -= 1;
 			data_size += pointers[i].length;
 			break;
 		case 'a':
@@ -105,7 +107,8 @@ bool msrpc_gen(TALLOC_CTX *mem_ctx,
 				return false;
 			}
 			pointers[i].length = n;
-			pointers[i].length -= 2;
+			if (pointers[i].length >= 2)
+				pointers[i].length -= 2;
 			data_size += pointers[i].length + 4;
 			break;
 		case 'B':
diff --git a/source3/lib/charcnv.c b/source3/lib/charcnv.c
index 5b2149b9..4b03132 100644
--- a/source3/lib/charcnv.c
+++ b/source3/lib/charcnv.c
@@ -429,7 +429,12 @@ size_t convert_string(charset_t from, charset_t to,
  *
  * Ensure the srclen contains the terminating zero.
  *
- * I hate the goto's in this function. It's embarressing.....
+ * FIXME
+ * The API here is a bit of a dogs breakfast: when iconv is successfully used
+ * for the conversion, converted_size includes the null terminator. Otherwise
+ * converted_size may be zero on success (srclen=0 or allow_bad_conv=true).
+ *
+ * I hate the goto's in this function. It's embarrassing.....
  * There has to be a cleaner way to do this. JRA.
  */
 bool convert_string_talloc(TALLOC_CTX *ctx, charset_t from, charset_t to,
-- 
1.7.3.4



More information about the samba-technical mailing list