ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
LS
Hello Lukas,
Patch looks OK to me, but I wonder if you could use \t for tabs? This code is pain already:-).
Thanks, PR
On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
LS Summary > Report jvFA5F Bug Summary File: /sssd/bdir/../src/util/util.c Location: line 208, column 22 Description: The left operand of '!=' is a garbage value Report Bug Annotated Source Code 1 /* 2 Authors: 3 Simo Sorce ssorce@redhat.com 4
5 Copyright (C) 2009 Red Hat 6
7 This program is free software; you can redistribute it and/or modify 8 it under the terms of the GNU General Public License as published by 9 the Free Software Foundation; either version 3 of the License, or 10 (at your option) any later version. 11
12 This program is distributed in the hope that it will be useful, 13 but WITHOUT ANY WARRANTY; without even the implied warranty of 14 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 15 GNU General Public License for more details. 16
17 You should have received a copy of the GNU General Public License 18 along with this program. If not, see http://www.gnu.org/licenses/. 19 */ 20
21 #include <ctype.h> 22 #include <netdb.h> 23 #include <poll.h> 24 #include <sys/socket.h> 25 #include <arpa/inet.h> 26 #include <talloc.h> 27 #include <dhash.h> 28
29 #include "util/util.h" 30 #include "util/sss_utf8.h" 31
32 int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, 33 const char sep, bool_Bool trim, bool_Bool skip_empty, 34 char ***_list, int *size) 35 { 36 int ret; 37 const char *substr_end = str; 38 const char *substr_begin = str; 39 const char *sep_pos = NULL((void*)0); 40 size_t substr_len; 41 char **list = NULL((void*)0); 42 int num_strings = 0; 43 TALLOC_CTX *tmp_ctx = NULL((void*)0); 44
45 if (str == NULL((void*)0) || *str == '\0' || _list == NULL((void*)0)) { 46 return EINVAL22; 47 } 48
49 tmp_ctx = talloc_new(NULL)talloc_named_const(((void*)0), 0, "talloc_new: " "../src/util/util.c" ":" "49"); 50 if (tmp_ctx == NULL((void*)0)) { 51 return ENOMEM12; 52 } 53
54 do { 55 substr_len = 0; 56
57 /* If this is not the first substring, then move from the separator. */ 58 if (sep_pos != NULL((void*)0)) { 59 substr_end = sep_pos + 1; 60 substr_begin = sep_pos + 1; 61 } 62
63 /* Find end of the first substring */ 64 while (*substr_end != sep && *substr_end != '\0') { 65 substr_end++; 66 substr_len++; 67 } 68
69 sep_pos = substr_end; 70
71 if (trim) { 72 /* Trim leading whitespace */ 73 while (isspace(*substr_begin)((*__ctype_b_loc ())[(int) ((*substr_begin))] & (unsigned short int) _ISspace) && substr_begin < substr_end) { 74 substr_begin++; 75 substr_len--; 76 } 77
78 /* Trim trailing whitespace */ 79 while (substr_end - 1 > substr_begin && isspace(*(substr_end-1))((*__ctype_b_loc ())[(int) ((*(substr_end-1)))] & (unsigned short int) _ISspace)) { 80 substr_end--; 81 substr_len--; 82 } 83 } 84
85 /* Copy the substring to the output list of strings */ 86 if (skip_empty == false0 || substr_len > 0) { 87 list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2)(char* *)_talloc_realloc_array(tmp_ctx, list, sizeof(char*), num_strings
- 2, "char*");
88 if (list == NULL((void*)0)) { 89 ret = ENOMEM12; 90 goto done; 91 } 92
93 /* empty string is stored for substr_len == 0 */ 94 list[num_strings] = talloc_strndup(list, substr_begin, substr_len); 95 if (list[num_strings] == NULL((void*)0)) { 96 ret = ENOMEM12; 97 goto done; 98 } 99 num_strings++; 100 } 101
102 } while (*sep_pos != '\0'); 103
104 if (list == NULL((void*)0)) { 105 /* No allocations were done, make space for the NULL */ 106 list = talloc(tmp_ctx, char *)(char
- *)talloc_named_const(tmp_ctx,
sizeof(char *), "char *" ); 107 if (list == NULL((void*)0)) { 108 ret = ENOMEM12; 109 goto done; 110 } 111 } 112 list[num_strings] = NULL((void*)0); 113
114 if (size) { 115 *size = num_strings; 116 } 117
118 *_list = talloc_steal(mem_ctx, list)({ __typeof__(list) __talloc_steal_ret = (__typeof__(list))_talloc_steal_loc ((mem_ctx),(list), "../src/util/util.c" ":" "118"); __talloc_steal_ret ; }); 119 ret = EOK0; 120 done: 121 talloc_free(tmp_ctx)_talloc_free(tmp_ctx, "../src/util/util.c" ":" "121"); 122 return ret; 123 } 124
125 static void free_args(char **args) 126 { 127 int i; 128
129 if (args) { 130 for (i = 0; args[i]; i++) free(args[i]); 131 free(args); 132 } 133 } 134
135 /* parse a string into arguments. 136
- arguments are separated by a
space 137
- '' is an escape character and
can be used only to escape 138
- itself or the white space.
139 */ 140 char **parse_args(const char *str) 141 { 142 const char *p; 143 char **ret, **r; 144 char *tmp; 145 int num; 146 int i; 147 bool_Bool e, w; 148
149 tmp = malloc(strlen(str) + 1); 150 if (!tmp) return NULL((void*)0);
1 Assuming 'tmp' is non-null →
2 ← Taking false branch → 151
152 ret = NULL((void*)0); 153 num = 0; 154 i = 0; 155 e = false0; 156 w = false0; 157 p = str; 158 while (*p) {
3 ← Loop condition is true. Entering loop body →
13 ← Loop condition is true. Entering loop body → 159 if (*p == '\') {
4 ← Taking false branch →
14 ← Taking true branch → 160 w = false0; 161 if (e) {
15 ← Taking false branch → 162 /* if we were already escaping, add a '' literal */ 163 tmp[i] = '\'; 164 i++; 165 e = false0; 166 } else { 167 /* otherwise just start escaping */ 168 e = true1; 169 } 170 } else if (isspace(*p)((*__ctype_b_loc ())[(int) ((*p))] & (unsigned short int) _ISspace)) {
5 ← Taking false branch → 171 if (e) { 172 /* Add escaped whitespace literally */ 173 tmp[i] = *p; 174 i++; 175 e = false0; 176 } else if (w == false0) { 177 /* If previous character was non-whitespace, arg break */ 178 tmp[i] = '\0'; 179 i++; 180 w = true1; 181 } 182 /* previous char was whitespace as well, skip it */ 183 } else { 184 w = false0; 185 if (e) {
6 ← Taking false branch → 186 /* Prepend escaped chars with a literal \ */ 187 tmp[i] = '\'; 188 i++; 189 e = false0; 190 } 191 /* Copy character from the source string */ 192 tmp[i] = *p; 193 i++; 194 } 195
196 p++; 197
198 /* check if this was the last char */ 199 if (*p == '\0') {
7 ← Taking true branch →
16 ← Taking false branch → 200 if (e) {
8 ← Taking false branch → 201 tmp[i] = '\'; 202 i++; 203 e = false0; 204 } 205 tmp[i] = '\0'; 206 i++; 207 } 208 if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
9 ← Taking false branch →
17 ← The left operand of '!=' is a garbage value 209 /* check next char and skip multiple spaces */ 210 continue; 211 } 212
213 r = realloc(ret, (num + 2) * sizeof(char *)); 214 if (!r) goto fail;
10 ← Assuming 'r' is non-null →
11 ← Taking false branch → 215 ret = r; 216 ret[num+1] = NULL((void*)0); 217 ret[num] = strdup(tmp); 218 if (!ret[num]) goto fail;
12 ← Taking false branch → 219 num++; 220 i = 0; 221 } 222
223 free(tmp); 224 return ret; 225
226 fail: 227 free(tmp); 228 free_args(ret); 229 return NULL((void*)0); 230 } 231
232 char **dup_string_list(TALLOC_CTX *memctx, const char **str_list) 233 { 234 int i = 0; 235 int j = 0; 236 char **dup_list; 237
238 if (!str_list) { 239 return NULL((void*)0); 240 } 241
242 /* Find the size of the list */ 243 while (str_list[i]) i++; 244
245 dup_list = talloc_array(memctx, char *, i+1)(char * *)_talloc_array(memctx, sizeof(char *), i+1, "char *" ); 246 if (!dup_list) { 247 return NULL((void*)0); 248 } 249
250 /* Copy the elements */ 251 for (j = 0; j < i; j++) { 252 dup_list[j] = talloc_strdup(dup_list, str_list[j]); 253 if (!dup_list[j]) { 254 talloc_free(dup_list)_talloc_free(dup_list, "../src/util/util.c" ":" "254"); 255 return NULL((void*)0); 256 } 257 } 258
259 /* NULL-terminate the list */ 260 dup_list[i] = NULL((void*)0); 261
262 return dup_list; 263 } 264
265 /* Take two string lists (terminated on a NULL char*) 266
- and return up to three arrays of
strings based on 267
- shared ownership.
268
269
- Pass NULL to any return type you
don't care about 270 */ 271 errno_t diff_string_lists(TALLOC_CTX *memctx, 272 char **_list1, 273 char **_list2, 274 char ***_list1_only, 275 char ***_list2_only, 276 char ***_both_lists) 277 { 278 int error; 279 errno_t ret; 280 int i; 281 int i2 = 0; 282 int i12 = 0; 283 hash_table_t *table; 284 hash_key_t key; 285 hash_value_t value; 286 char **list1 = NULL((void*)0); 287 char **list2 = NULL((void*)0); 288 char **list1_only = NULL((void*)0); 289 char **list2_only = NULL((void*)0); 290 char **both_lists = NULL((void*)0); 291 unsigned long count; 292 hash_key_t *keys; 293
294 TALLOC_CTX *tmp_ctx = talloc_new(memctx)talloc_named_const(memctx, 0, "talloc_new: " "../src/util/util.c" ":" "294"); 295 if (!tmp_ctx) { 296 return ENOMEM12; 297 } 298
299 if (!_list1) { 300 list1 = talloc_array(tmp_ctx, char *, 1)(char * *)_talloc_array(tmp_ctx, sizeof(char *), 1, "char *"); 301 if (!list1) { 302 talloc_free(tmp_ctx)_talloc_free(tmp_ctx, "../src/util/util.c" ":" "302"); 303 return ENOMEM12; 304 } 305 list1[0] = NULL((void*)0); 306 } 307 else { 308 list1 = _list1; 309 } 310
311 if (!_list2) { 312 list2 = talloc_array(tmp_ctx, char *, 1)(char * *)_talloc_array(tmp_ctx, sizeof(char *), 1, "char *"); 313 if (!list2) { 314 talloc_free(tmp_ctx)_talloc_free(tmp_ctx, "../src/util/util.c" ":" "314"); 315 return ENOMEM12; 316 } 317 list2[0] = NULL((void*)0); 318 } 319 else { 320 list2 = _list2; 321 } 322
323 error = hash_create(10, &table, NULL((void*)0), NULL((void*)0)); 324 if (error != HASH_SUCCESS0) { 325 talloc_free(tmp_ctx)_talloc_free(tmp_ctx, "../src/util/util.c" ":" "325"); 326 return EIO5; 327 } 328
329 key.type = HASH_KEY_STRING; 330 value.type = HASH_VALUE_UNDEF; 331
332 /* Add all entries from list 1 into a hash table */ 333 i = 0; 334 while (list1[i]) { 335 key.str = talloc_strdup(tmp_ctx, list1[i]); 336 error = hash_enter(table, &key, &value); 337 if (error != HASH_SUCCESS0) { 338 ret = EIO5; 339 goto done; 340 } 341 i++; 342 } 343
344 /* Iterate through list 2 and remove matching items */ 345 i = 0; 346 while (list2[i]) { 347 key.str = talloc_strdup(tmp_ctx, list2[i]); 348 error = hash_delete(table, &key); 349 if (error == HASH_SUCCESS0) { 350 if (_both_lists) { 351 /* String was present in both lists */ 352 i12++; 353 both_lists = talloc_realloc(tmp_ctx, both_lists, char *, i12+1)(char * *)_talloc_realloc_array(tmp_ctx, both_lists, sizeof(char *), i12+1, "char *"); 354 if (!both_lists) { 355 ret = ENOMEM12; 356 goto done; 357 } 358 both_lists[i12-1] = talloc_strdup(both_lists, list2[i]); 359 if (!both_lists[i12-1]) { 360 ret = ENOMEM12; 361 goto done; 362 } 363
364 both_lists[i12] = NULL((void*)0); 365 } 366 } 367 else if (error == HASH_ERROR_KEY_NOT_FOUND(-2000 + 4)) { 368 if (_list2_only) { 369 /* String was present only in list2 */ 370 i2++; 371 list2_only = talloc_realloc(tmp_ctx, list2_only,(char * *)_talloc_realloc_array(tmp_ctx, list2_only, sizeof(char *), i2+1, "char *") 372 char *, i2+1)(char * *)_talloc_realloc_array(tmp_ctx, list2_only, sizeof(char *), i2+1, "char *"); 373 if (!list2_only) { 374 ret = ENOMEM12; 375 goto done; 376 } 377 list2_only[i2-1] = talloc_strdup(list2_only, list2[i]); 378 if (!list2_only[i2-1]) { 379 ret = ENOMEM12; 380 goto done; 381 } 382
383 list2_only[i2] = NULL((void*)0); 384 } 385 } 386 else { 387 /* An error occurred */ 388 ret = EIO5; 389 goto done; 390 } 391 i++; 392 } 393
394 /* Get the leftover entries in the hash table */ 395 if (_list1_only) { 396 error = hash_keys(table, &count, &keys); 397 if (error != HASH_SUCCESS0) { 398 ret = EIO5; 399 goto done; 400 } 401
402 list1_only = talloc_array(tmp_ctx, char *, count+1)(char * *)_talloc_array(tmp_ctx, sizeof(char *), count+1, "char *" ); 403 if (!list1_only) { 404 ret = ENOMEM12; 405 goto done; 406 } 407
408 for (i = 0; i < count; i++) { 409 list1_only[i] = talloc_strdup(list1_only, keys[i].str); 410 if (!list1_only[i]) { 411 ret = ENOMEM12; 412 goto done; 413 } 414 } 415 list1_only[count] = NULL((void*)0); 416
417 free(keys); 418
419 *_list1_only = talloc_steal(memctx, list1_only)({ __typeof__(list1_only) __talloc_steal_ret = (__typeof__(list1_only ))_talloc_steal_loc((memctx),(list1_only), "../src/util/util.c" ":" "419"); __talloc_steal_ret; }); 420 } 421
422 if (_list2_only) { 423 if (list2_only) { 424 *_list2_only = talloc_steal(memctx, list2_only)({ __typeof__(list2_only) __talloc_steal_ret = (__typeof__(list2_only ))_talloc_steal_loc((memctx),(list2_only), "../src/util/util.c" ":" "424"); __talloc_steal_ret; }); 425 } 426 else { 427 *_list2_only = talloc_array(memctx, char *, 1)(char * *)_talloc_array(memctx, sizeof(char *), 1, "char *"); 428 if (!(*_list2_only)) { 429 ret = ENOMEM12; 430 goto done; 431 } 432 *_list2_only[0] = NULL((void*)0); 433 } 434 } 435
436 if (_both_lists) { 437 if (both_lists) { 438 *_both_lists = talloc_steal(memctx, both_lists)({ __typeof__(both_lists) __talloc_steal_ret = (__typeof__(both_lists ))_talloc_steal_loc((memctx),(both_lists), "../src/util/util.c" ":" "438"); __talloc_steal_ret; }); 439 } 440 else { 441 *_both_lists = talloc_array(memctx, char *, 1)(char * *)_talloc_array(memctx, sizeof(char *), 1, "char *"); 442 if (!(*_both_lists)) { 443 ret = ENOMEM12; 444 goto done; 445 } 446 *_both_lists[0] = NULL((void*)0); 447 } 448 } 449
450 ret = EOK0; 451
452 done: 453 hash_destroy(table); 454 talloc_free(tmp_ctx)_talloc_free(tmp_ctx, "../src/util/util.c" ":" "454"); 455 return ret; 456 } 457
458 static void *hash_talloc(const size_t size, void *pvt) 459 { 460 return talloc_size(pvt, size)talloc_named_const(pvt, size, "../src/util/util.c" ":" "460"); 461 } 462
463 static void hash_talloc_free(void *ptr, void *pvt) 464 { 465 talloc_free(ptr)_talloc_free(ptr, "../src/util/util.c" ":" "465"); 466 } 467
468 errno_t sss_hash_create_ex(TALLOC_CTX *mem_ctx, 469 unsigned long count, 470 hash_table_t **tbl, 471 unsigned int directory_bits, 472 unsigned int segment_bits, 473 unsigned long min_load_factor, 474 unsigned long max_load_factor, 475 hash_delete_callback *delete_callback, 476 void *delete_private_data) 477 { 478 errno_t ret; 479 hash_table_t *table; 480 int hret; 481
482 TALLOC_CTX *internal_ctx; 483 internal_ctx = talloc_new(NULL)talloc_named_const(((void*)0), 0, "talloc_new: " "../src/util/util.c" ":" "483"); 484 if (!internal_ctx) { 485 return ENOMEM12; 486 } 487
488 hret = hash_create_ex(count, &table, directory_bits, segment_bits, 489 min_load_factor, max_load_factor, 490 hash_talloc, hash_talloc_free, internal_ctx, 491 delete_callback, delete_private_data); 492 switch (hret) { 493 case HASH_SUCCESS0: 494 /* Steal the table pointer onto the mem_ctx, 495
- then make the internal_ctx a
child of 496
- table.
497
498
- This way, we can clean up the
values when 499
- we talloc_free() the table
500 */ 501 *tbl = talloc_steal(mem_ctx, table)({ __typeof__(table) __talloc_steal_ret = (__typeof__(table)) _talloc_steal_loc((mem_ctx),(table), "../src/util/util.c" ":" "501"); __talloc_steal_ret; }); 502 talloc_steal(table, internal_ctx)({ __typeof__(internal_ctx) __talloc_steal_ret = (__typeof__( internal_ctx))_talloc_steal_loc((table),(internal_ctx), "../src/util/util.c" ":" "502"); __talloc_steal_ret; }); 503 return EOK0; 504
505 case HASH_ERROR_NO_MEMORY(-2000 + 3): 506 ret = ENOMEM12; 507 break; 508 default: 509 ret = EIO5; 510 } 511
512 DEBUG(SSSDBG_FATAL_FAILURE, "Could not create hash table: [%d][% s]\n",do { int __debug_macro_level = 0x0010; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 513, __FUNCTION__, __debug_macro_level, "Could not create hash table: [%d][%s]\n" , hret, hash_error_string(hret)); } } while (0) 513 hret, hash_error_string(hret))do { int __debug_macro_level = 0x0010; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 513, __FUNCTION__, __debug_macro_level, "Could not create hash table: [%d][%s]\n" , hret, hash_error_string(hret)); } } while (0); 514
515 talloc_free(internal_ctx)_talloc_free(internal_ctx, "../src/util/util.c" ":" "515"); 516 return ret; 517 } 518
519 errno_t sss_hash_create(TALLOC_CTX *mem_ctx, unsigned long count, 520 hash_table_t **tbl) 521 { 522 return sss_hash_create_ex(mem_ctx, count, tbl, 0, 0, 0, 0, NULL((void*)0), NULL((void*)0)); 523 } 524
525 errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, 526 const char *input, 527 char **sanitized) 528 { 529 char *output; 530 size_t i = 0; 531 size_t j = 0; 532
533 /* Assume the worst-case. We'll resize it later, once */ 534 output = talloc_array(mem_ctx, char, strlen(input) * 3 + 1)(char *)_talloc_array(mem_ctx, sizeof(char), strlen(input) * 3
- 1, "char");
535 if (!output) { 536 return ENOMEM12; 537 } 538
539 while (input[i]) { 540 switch(input[i]) { 541 case '\t': 542 output[j++] = '\'; 543 output[j++] = '0'; 544 output[j++] = '9'; 545 break; 546 case ' ': 547 output[j++] = '\'; 548 output[j++] = '2'; 549 output[j++] = '0'; 550 break; 551 case '*': 552 output[j++] = '\'; 553 output[j++] = '2'; 554 output[j++] = 'a'; 555 break; 556 case '(': 557 output[j++] = '\'; 558 output[j++] = '2'; 559 output[j++] = '8'; 560 break; 561 case ')': 562 output[j++] = '\'; 563 output[j++] = '2'; 564 output[j++] = '9'; 565 break; 566 case '\': 567 output[j++] = '\'; 568 output[j++] = '5'; 569 output[j++] = 'c'; 570 break; 571 default: 572 output[j++] = input[i]; 573 } 574
575 i++; 576 } 577 output[j] = '\0'; 578 *sanitized = talloc_realloc(mem_ctx, output, char, j+1)(char *)_talloc_realloc_array(mem_ctx, output, sizeof(char), j +1, "char"); 579 if (!*sanitized) { 580 talloc_free(output)_talloc_free(output, "../src/util/util.c" ":" "580"); 581 return ENOMEM12; 582 } 583
584 return EOK0; 585 } 586
587 char * 588 sss_escape_ip_address(TALLOC_CTX *mem_ctx, int family, const char *addr) 589 { 590 return family == AF_INET610 ? talloc_asprintf(mem_ctx, "[%s]", addr) : 591 talloc_strdup(mem_ctx, addr); 592 } 593
594 /* out->len includes terminating '\0' */ 595 void to_sized_string(struct sized_string *out, const char *in) 596 { 597 out->str = in; 598 if (out->str) { 599 out->len = strlen(out->str) + 1; 600 } else { 601 out->len = 0; 602 } 603 } 604
605 /* This function only removes first and last 606
- character if the first character
was '['. 607
608
- NOTE: This means, that ipv6addr
must NOT be followed 609
- by port number.
610 */ 611 errno_t 612 remove_ipv6_brackets(char *ipv6addr) 613 { 614 size_t len; 615
616 if (ipv6addr && ipv6addr[0] == '[') { 617 len = strlen(ipv6addr); 618 if (len < 3) { 619 return EINVAL22; 620 } 621
622 memmove(ipv6addr, &ipv6addr[1], len
- 2);
623 ipv6addr[len -2] = '\0'; 624 } 625
626 return EOK0; 627 } 628
629 errno_t add_string_to_list(TALLOC_CTX *mem_ctx, const char *string, 630 char ***list_p) 631 { 632 size_t c; 633 char **old_list = NULL((void*)0); 634 char **new_list = NULL((void*)0); 635
636 if (string == NULL((void*)0) || list_p == NULL((void*)0)) { 637 DEBUG(SSSDBG_OP_FAILURE, "Missing string or list.\n")do { int __debug_macro_level = 0x0040; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 637, __FUNCTION__, __debug_macro_level, "Missing string or list.\n" ); } } while (0); 638 return EINVAL22; 639 } 640
641 old_list = *list_p; 642
643 if (old_list == NULL((void*)0)) { 644 /* If the input is a NULL list a new one is created with the new 645
- string and the terminating NULL
element. */ 646 c = 0; 647 new_list = talloc_array(mem_ctx, char *, 2)(char * *)_talloc_array(mem_ctx, sizeof(char *), 2, "char *"); 648 } else { 649 for (c = 0; old_list[c] != NULL((void*)0); c++); 650 /* Allocate one extra space for the new service and one for 651
- the terminating NULL
652 */ 653 new_list = talloc_realloc(mem_ctx, old_list, char *, c + 2)(char * *)_talloc_realloc_array(mem_ctx, old_list, sizeof(char *), c + 2, "char *"); 654 } 655
656 if (new_list == NULL((void*)0)) { 657 DEBUG(SSSDBG_OP_FAILURE, "talloc_array/talloc_realloc failed.\n")do { int __debug_macro_level = 0x0040; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 657, __FUNCTION__, __debug_macro_level, "talloc_array/talloc_realloc failed.\n" ); } } while (0); 658 return ENOMEM12; 659 } 660
661 new_list[c] = talloc_strdup(new_list, string); 662 if (new_list[c] == NULL((void*)0)) { 663 DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n")do { int __debug_macro_level = 0x0040; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 663, __FUNCTION__, __debug_macro_level, "talloc_strdup failed.\n" ); } } while (0); 664 talloc_free(new_list)_talloc_free(new_list, "../src/util/util.c" ":" "664"); 665 return ENOMEM12; 666 } 667
668 new_list[c + 1] = NULL((void*)0); 669
670 *list_p = new_list; 671
672 return EOK0; 673 } 674
675 bool_Bool string_in_list(const char *string, char **list, bool_Bool case_sensitive) 676 { 677 size_t c; 678 int(*compare)(const char *s1, const char *s2); 679
680 if (string == NULL((void*)0) || list == NULL((void*)0) || *list == NULL((void*)0)) { 681 return false0; 682 } 683
684 compare = case_sensitive ? strcmp : strcasecmp; 685
686 for (c = 0; list[c] != NULL((void*)0); c++) { 687 if (compare(string, list[c]) == 0) { 688 return true1; 689 } 690 } 691
692 return false0; 693 } 694
695 void safezero(void *data, size_t size) 696 { 697 volatile uint8_t *p = data; 698
699 while (size--) { 700 *p++ = 0; 701 } 702 } 703
704 int domain_to_basedn(TALLOC_CTX *memctx, const char *domain, char **basedn) 705 { 706 const char *s; 707 char *dn; 708 char *p; 709 int l; 710
711 if (!domain || !basedn) { 712 return EINVAL22; 713 } 714
715 s = domain; 716 dn = talloc_strdup(memctx, "dc="); 717
718 while ((p = strchr(s, '.'))) { 719 l = p - s; 720 dn = talloc_asprintf_append_buffer(dn, "%.*s,dc=", l, s); 721 if (!dn) { 722 return ENOMEM12; 723 } 724 s = p + 1; 725 } 726 dn = talloc_strdup_append_buffer(dn, s); 727 if (!dn) { 728 return ENOMEM12; 729 } 730
731 for (p=dn; *p; ++p) { 732 *p = tolower(*p); 733 } 734
735 *basedn = dn; 736 return EOK0; 737 } 738
739 bool_Bool is_host_in_domain(const char *host, const char *domain) 740 { 741 int diff = strlen(host) - strlen(domain); 742
743 if (diff == 0 && strcmp(host, domain) == 0) { 744 return true1; 745 } 746
747 if (diff > 0 && strcmp(host + diff, domain) == 0 && host[diff - 1] == '.') { 748 return true1; 749 } 750
751 return false0; 752 } 753
754 /* addr is in network order for both IPv4 and IPv6 versions */ 755 bool_Bool check_ipv4_addr(struct in_addr *addr, uint8_t flags) 756 { 757 char straddr[INET_ADDRSTRLEN16]; 758
759 if (inet_ntop(AF_INET2, addr, straddr, INET_ADDRSTRLEN16) == NULL((void*)0)) { 760 DEBUG(SSSDBG_MINOR_FAILURE,do { int __debug_macro_level = 0x0080; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 761, __FUNCTION__, __debug_macro_level, "inet_ntop failed, won't log IP addresses\n" ); } } while (0) 761 "inet_ntop failed, won't log IP addresses\n")do { int __debug_macro_level = 0x0080; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 761, __FUNCTION__, __debug_macro_level, "inet_ntop failed, won't log IP addresses\n" ); } } while (0); 762 snprintf(straddr, INET_ADDRSTRLEN16, "unknown"); 763 } 764
765 if ((flags & SSS_NO_MULTICAST0x04) && IN_MULTICAST(ntohl(addr->s_addr))((((in_addr_t)(ntohl(addr->s_addr))) & 0xf0000000) == 0xe0000000 )) { 766 DEBUG(SSSDBG_FUNC_DATA, "Multicast IPv4 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 766, __FUNCTION__, __debug_macro_level, "Multicast IPv4 address %s\n" , straddr); } } while (0); 767 return false0; 768 } else if ((flags & SSS_NO_LOOPBACK0x02) 769 && inet_netof(*addr) == IN_LOOPBACKNET127) { 770 DEBUG(SSSDBG_FUNC_DATA, "Loopback IPv4 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 770, __FUNCTION__, __debug_macro_level, "Loopback IPv4 address %s\n" , straddr); } } while (0); 771 return false0; 772 } else if ((flags & SSS_NO_LINKLOCAL0x01) 773 && (addr->s_addr & htonl(0xffff0000)) == htonl(0xa9fe0000)) { 774 /* 169.254.0.0/16 */ 775 DEBUG(SSSDBG_FUNC_DATA, "Link-local IPv4 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 775, __FUNCTION__, __debug_macro_level, "Link-local IPv4 address %s\n" , straddr); } } while (0); 776 return false0; 777 } else if ((flags & SSS_NO_BROADCAST0x08) 778 && addr->s_addr == htonl(INADDR_BROADCAST((in_addr_t) 0xffffffff))) { 779 DEBUG(SSSDBG_FUNC_DATA, "Broadcast IPv4 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 779, __FUNCTION__, __debug_macro_level, "Broadcast IPv4 address %s\n" , straddr); } } while (0); 780 return false0; 781 } 782
783 return true1; 784 } 785
786 bool_Bool check_ipv6_addr(struct in6_addr *addr, uint8_t flags) 787 { 788 char straddr[INET6_ADDRSTRLEN46]; 789
790 if (inet_ntop(AF_INET610, addr, straddr, INET6_ADDRSTRLEN46) == NULL((void*)0)) { 791 DEBUG(SSSDBG_MINOR_FAILURE,do { int __debug_macro_level = 0x0080; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 792, __FUNCTION__, __debug_macro_level, "inet_ntop failed, won't log IP addresses\n" ); } } while (0) 792 "inet_ntop failed, won't log IP addresses\n")do { int __debug_macro_level = 0x0080; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 792, __FUNCTION__, __debug_macro_level, "inet_ntop failed, won't log IP addresses\n" ); } } while (0); 793 snprintf(straddr, INET6_ADDRSTRLEN46, "unknown"); 794 } 795
796 if ((flags & SSS_NO_LINKLOCAL0x01) && IN6_IS_ADDR_LINKLOCAL(addr)(__extension__ ({ const struct in6_addr *__a = (const struct in6_addr *) (addr); (__a->__in6_u.__u6_addr32[0] & htonl (0xffc00000 )) == htonl (0xfe800000); }))) { 797 DEBUG(SSSDBG_FUNC_DATA, "Link local IPv6 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 797, __FUNCTION__, __debug_macro_level, "Link local IPv6 address %s\n" , straddr); } } while (0); 798 return false0; 799 } else if ((flags & SSS_NO_LOOPBACK0x02) && IN6_IS_ADDR_LOOPBACK(addr)(__extension__ ({ const struct in6_addr *__a = (const struct in6_addr *) (addr); __a->__in6_u.__u6_addr32[0] == 0 && __a ->__in6_u.__u6_addr32[1] == 0 && __a->__in6_u.__u6_addr32 [2] == 0 && __a->__in6_u.__u6_addr32[3] == htonl ( 1); }))) { 800 DEBUG(SSSDBG_FUNC_DATA, "Loopback IPv6 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 800, __FUNCTION__, __debug_macro_level, "Loopback IPv6 address %s\n" , straddr); } } while (0); 801 return false0; 802 } else if ((flags & SSS_NO_MULTICAST0x04) && IN6_IS_ADDR_MULTICAST(addr)(((const uint8_t *) (addr))[0] == 0xff)) { 803 DEBUG(SSSDBG_FUNC_DATA, "Multicast IPv6 address %s\n", straddr)do { int __debug_macro_level = 0x0200; if ((debug_level & (__debug_macro_level) || (debug_level == 0 && (__debug_macro_level & (0x0010 | 0x0020))))) { debug_fn("../src/util/util.c", 803, __FUNCTION__, __debug_macro_level, "Multicast IPv6 address %s\n" , straddr); } } while (0); 804 return false0; 805 } 806
807 return true1; 808 } 809
810 const char * const * get_known_services(void) 811 { 812 static const char *svc[] = {"nss", "pam", "sudo", "autofs", 813 "ssh", "pac", "ifp", NULL((void*)0) }; 814
815 return svc; 816 } _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (24/06/14 18:07), Pavel Reichl wrote:
Hello Lukas,
Patch looks OK to me, but I wonder if you could use \t for tabs? This code is pain already:-).
I strongly disagree. Unit test is not terible code. It's nice.
Previously I didn't want to use '\t' instead of tabs because there is lof of backslashes anyway.
Let's compare diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c index 5ef0102..4f47305 100644 --- a/src/tests/util-tests.c +++ b/src/tests/util-tests.c @@ -142,7 +142,7 @@ START_TEST(test_parse_args) const char *parsed7[] = { "foo", "a", "f", NULL }; const char *parsed8[] = { "foo", "a\tg", NULL }; const char *parsed9[] = { "foo", NULL }; - const char *parsed10[] = { " ", "foo", " ", "\'", NULL }; + const char *parsed10[] = { " ", "foo", "\t", "\'", NULL }; const char *parsed11[] = { "a", NULL }; struct pa_testcase tc[] = { { "foo", parsed1 }, @@ -151,14 +151,14 @@ START_TEST(test_parse_args) { "foo a\ c", parsed4 }, { "foo a d ", parsed5 }, { "foo a e ", parsed6 }, - { "foo a f ", parsed7 }, + { "foo\ta\t \tf \t", parsed7 }, { "foo a\\tg", parsed8 }, { " foo ", parsed9 }, - { "\ foo \ \' ", parsed10 }, + { "\ foo \\t \' ", parsed10 }, { "a", parsed11 }, { " ", NULL }, { "", NULL }, - { " ", NULL }, + { " \t ", NULL }, { NULL, NULL } };
Each normal editor can show difference between tabs and spaces IMO, It looks to me like obfluscation after replacement.
If you really want to do this change I can do it. I don't mind :-)
LS
PS: In future, try to avoid including html attachments into replies.
Summary > Report jvFA5F Bug Summary File: /sssd/bdir/../src/util/util.c Location: line 208, column 22 Description: The left operand of '!=' is a garbage value Report Bug Annotated Source Code 1 /* 2 Authors: 3 Simo Sorce ssorce@redhat.com 4
5 Copyright (C) 2009 Red Hat 6
7 This program is free software; you can redistribute it and/or modify 8 it under the terms of the GNU General Public License as published by 9 the Free Software Foundation; either version 3 of the License, or 10 (at your option) any later version. 11
12 This program is distributed in the hope that it will be useful, 13 but WITHOUT ANY WARRANTY; without even the implied warranty of 14 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 15 GNU General Public License for more details. 16
17 You should have received a copy of the GNU General Public License 18 along with this program. If not, see http://www.gnu.org/licenses/. 19 */ 20
//2160 lines removed.
On Wed, 2014-06-25 at 09:54 +0200, Lukas Slebodnik wrote:
On (24/06/14 18:07), Pavel Reichl wrote:
Hello Lukas,
Patch looks OK to me, but I wonder if you could use \t for tabs? This code is pain already:-).
I strongly disagree. Unit test is not terible code. It's nice.
I didn't mean to insult your or anybody else's code. The UT and especially parse_args are just difficult for me to comprehend. I thought you also had hard time fixing parse_args, hadn't you?
Previously I didn't want to use '\t' instead of tabs because there is lof of backslashes anyway.
Let's compare diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c index 5ef0102..4f47305 100644 --- a/src/tests/util-tests.c +++ b/src/tests/util-tests.c @@ -142,7 +142,7 @@ START_TEST(test_parse_args) const char *parsed7[] = { "foo", "a", "f", NULL }; const char *parsed8[] = { "foo", "a\tg", NULL }; const char *parsed9[] = { "foo", NULL };
- const char *parsed10[] = { " ", "foo", " ", "\'", NULL };
- const char *parsed10[] = { " ", "foo", "\t", "\'", NULL }; const char *parsed11[] = { "a", NULL }; struct pa_testcase tc[] = { { "foo", parsed1 },
@@ -151,14 +151,14 @@ START_TEST(test_parse_args) { "foo a\ c", parsed4 }, { "foo a d ", parsed5 }, { "foo a e ", parsed6 },
{ "foo a f ", parsed7 },
{ "foo\ta\t \tf \t", parsed7 }, { "foo a\\\tg", parsed8 }, { " foo ", parsed9 },
{ "\\ foo \\ \\' ", parsed10 },
{ "\\ foo \\\t \\' ", parsed10 }, { "a", parsed11 }, { " ", NULL }, { "", NULL },
{ " ", NULL },
};{ " \t ", NULL }, { NULL, NULL }
Each normal editor can show difference between tabs and spaces IMO, It looks to me like obfluscation after replacement.
I personally prefer \t version, but I respect your opinion so unless somebody else prefers explicit \t too, please keep you original version as is.
If you really want to do this change I can do it. I don't mind :-)
LS
PS: In future, try to avoid including html attachments into replies.
I'm sorry for that. Has this happen ever before or was this single incident annoying so much you needed to stress it out?
Summary > Report jvFA5F Bug Summary File: /sssd/bdir/../src/util/util.c Location: line 208, column 22 Description: The left operand of '!=' is a garbage value Report Bug Annotated Source Code 1 /* 2 Authors: 3 Simo Sorce ssorce@redhat.com 4
5 Copyright (C) 2009 Red Hat 6
7 This program is free software; you can redistribute it and/or modify 8 it under the terms of the GNU General Public License as published by 9 the Free Software Foundation; either version 3 of the License, or 10 (at your option) any later version. 11
12 This program is distributed in the hope that it will be useful, 13 but WITHOUT ANY WARRANTY; without even the implied warranty of 14 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 15 GNU General Public License for more details. 16
17 You should have received a copy of the GNU General Public License 18 along with this program. If not, see http://www.gnu.org/licenses/. 19 */ 20
//2160 lines removed. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 06/25/2014 11:35 AM, Pavel Reichl wrote:
On Wed, 2014-06-25 at 09:54 +0200, Lukas Slebodnik wrote:
Previously I didn't want to use '\t' instead of tabs because there is lof of backslashes anyway.
Let's compare diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c index 5ef0102..4f47305 100644 --- a/src/tests/util-tests.c +++ b/src/tests/util-tests.c @@ -142,7 +142,7 @@ START_TEST(test_parse_args) const char *parsed7[] = { "foo", "a", "f", NULL }; const char *parsed8[] = { "foo", "a\tg", NULL }; const char *parsed9[] = { "foo", NULL };
- const char *parsed10[] = { " ", "foo", " ", "\'", NULL };
- const char *parsed10[] = { " ", "foo", "\t", "\'", NULL }; const char *parsed11[] = { "a", NULL }; struct pa_testcase tc[] = { { "foo", parsed1 },
@@ -151,14 +151,14 @@ START_TEST(test_parse_args) { "foo a\ c", parsed4 }, { "foo a d ", parsed5 }, { "foo a e ", parsed6 },
{ "foo a f ", parsed7 },
{ "foo\ta\t \tf \t", parsed7 }, { "foo a\\\tg", parsed8 }, { " foo ", parsed9 },
{ "\\ foo \\ \\' ", parsed10 },
{ "\\ foo \\\t \\' ", parsed10 }, { "a", parsed11 }, { " ", NULL }, { "", NULL },
{ " ", NULL },
{ " \t ", NULL }, { NULL, NULL } };
Each normal editor can show difference between tabs and spaces IMO, It looks to me like obfluscation after replacement.
I personally prefer \t version, but I respect your opinion so unless somebody else prefers explicit \t too, please keep you original version as is.
The slash forest sure looks ugly, but I would prefer \t's as well, as they make the code less ambiguous and somewhat easier to read. I generally avoid enabling tab highlighting in the editor, as it makes code using them look too noisy.
Nick
On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
ACK to the patch as is although I would appreciate if you considered my and Nick's comments about '\t' and following nitpicks:
if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
/* check next char and skip multiple spaces */
continue;
}
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
num++;
i = 0;
/* save token to result array */
if (i>1 && tmp[i-1] == '\0') {
I think it's way more usual in SSSD code base to put space before and after '>' operator.
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
Would you consider replacing !r with r == NULL. I agree it is not in our code style guide, but I think most of new code use this form.
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
Same as above.
num++;
i = 0;
}}
Thanks.
On Wed, Jun 25, 2014 at 12:42:01PM +0200, Pavel Reichl wrote:
On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
ACK to the patch as is although I would appreciate if you considered my and Nick's comments about '\t' and following nitpicks:
I agree with the tab comment, too. Code readability should not depend on the editor settings.
if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
/* check next char and skip multiple spaces */
continue;
}
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
num++;
i = 0;
/* save token to result array */
if (i>1 && tmp[i-1] == '\0') {
I think it's way more usual in SSSD code base to put space before and after '>' operator.
+1
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
Would you consider replacing !r with r == NULL. I agree it is not in our code style guide, but I think most of new code use this form.
+1
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
Same as above.
+1
num++;
i = 0;
}}
Thanks.
On (25/06/14 12:42), Pavel Reichl wrote:
On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
ACK to the patch as is although I would appreciate if you considered my and Nick's comments about '\t' and following nitpicks:
As I already wrote. I don't mind. I was just waiting for common agreement.
if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
/* check next char and skip multiple spaces */
continue;
}
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
num++;
i = 0;
/* save token to result array */
if (i>1 && tmp[i-1] == '\0') {
I think it's way more usual in SSSD code base to put space before and after '>' operator.
Fixed.
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
Would you consider replacing !r with r == NULL. I agree it is not in our code style guide, but I think most of new code use this form.
No. It is not against coding style [1] http://www.freeipa.org/page/Coding_Style
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
Same as above.
No. I asked for a change in Coding Style few months ago [2]. Nothing happended. Simo wrote few times that this kind of test is acceptable for pointers after memory allocation.
num++;
i = 0;
}}
Thanks.
Thank you for review.
New version is attached.
LS [1] http://www.freeipa.org/page/Coding_Style [2] https://lists.fedorahosted.org/pipermail/sssd-devel/2014-March/018814.html
On Wed, Jun 25, 2014 at 02:09:24PM +0200, Lukas Slebodnik wrote:
On (25/06/14 12:42), Pavel Reichl wrote:
On Mon, 2014-06-23 at 09:13 +0200, Lukas Slebodnik wrote:
ehlo,
While parsing string with multiple whitespaces, it may happen variable i is zero and we want to test end of argument "tmp[i-1] != '\0'". Side effect of this bug is duplicite string output array. Input string: "foo b" Expected output: { "foo", "a", NULL } Output: { "foo", "foo", "a", NULL }
This patch uses inverted logic. Instead of testing whether to read next char or skip multiple whitespaces, we will test whether we have new argument which should be stored in output array.
How to test? libtool --mode=execute valgrind ./util-tests Running suite(s): util ==17839== Invalid read of size 1 ==17839== at 0x4E60329: parse_args (util.c:208) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839== Address 0x62f709f is 1 bytes before a block of size 7 alloc'd ==17839== at 0x4A0645D: malloc (vg_replace_malloc.c:291) ==17839== by 0x4E6020E: parse_args (util.c:149) ==17839== by 0x403A04: test_parse_args (util-tests.c:166) ==17839== by 0x4C2F139: tcase_run_tfun_nofork.isra.11 (check_run.c:327) ==17839== by 0x4C2F3C5: srunner_run (check_run.c:187) ==17839== by 0x4057ED: main (util-tests.c:1090) ==17839==
This problem was also reported by clang static analysers. We thought it was false positive. Unfortunately, it wasn't.
ACK to the patch as is although I would appreciate if you considered my and Nick's comments about '\t' and following nitpicks:
As I already wrote. I don't mind. I was just waiting for common agreement.
if (tmp[i-1] != '\0' || strlen(tmp) == 0) {
/* check next char and skip multiple spaces */
continue;
}
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
num++;
i = 0;
/* save token to result array */
if (i>1 && tmp[i-1] == '\0') {
I think it's way more usual in SSSD code base to put space before and after '>' operator.
Fixed.
r = realloc(ret, (num + 2) * sizeof(char *));
if (!r) goto fail;
Would you consider replacing !r with r == NULL. I agree it is not in our code style guide, but I think most of new code use this form.
No. It is not against coding style [1] http://www.freeipa.org/page/Coding_Style
You're right we shouldn't nack the patch on any formal grounds. For the record I agree with: https://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/
ret = r;
ret[num+1] = NULL;
ret[num] = strdup(tmp);
if (!ret[num]) goto fail;
Same as above.
No. I asked for a change in Coding Style few months ago [2]. Nothing happended. Simo wrote few times that this kind of test is acceptable for pointers after memory allocation.
num++;
i = 0;
}}
Thanks.
Thank you for review.
New version is attached.
The style issues were fixed. Unit tests pass, warnings are gone and the parse_args functionality seems to still work fine (I tested some different options and running valgrind for instance)
ACK
On Tue, Jul 08, 2014 at 09:50:13AM +0200, Jakub Hrozek wrote:
New version is attached.
The style issues were fixed. Unit tests pass, warnings are gone and the parse_args functionality seems to still work fine (I tested some different options and running valgrind for instance)
ACK
master: 852722ecb5dc09fc80cd3c837edb1cf6db529210
sssd-devel@lists.fedorahosted.org