Skip to content

liblsd: fix const-correctness in _parse_single_range() - #216

Merged
garlick merged 1 commit into
chaos:masterfrom
hector-cao:fix-const-ness
May 29, 2026
Merged

garlick merged 1 commit into
chaos:masterfrom
hector-cao:fix-const-ness

Conversation

@hector-cao

Copy link
Copy Markdown
Contributor

_parse_single_range() accepts a const char *str argument and creates a mutable copy via strdup() into orig. However, it was incorrectly calling strchr() and strtoul() on the original const pointer str rather than on the mutable copy.

This is both a correctness bug and a build failure with modern glibc:

  • glibc now provides const-preserving overloads of strchr(), returning const char * when passed a const char *. Assigning this to char *p discards the const qualifier, triggering a compile error with -Werror=discarded-qualifiers.
  • The subsequent *p++ = '\0' write through p would modify memory via a pointer originally derived from a const string.

Fix by using orig (the mutable strdup copy) for strchr() and strtoul() calls, which is the correct buffer to mutate.

@garlick

garlick commented May 28, 2026

Copy link
Copy Markdown
Member

Good catch!

However, a bunch of CI tests are failing with this change, for example:

$ ./t0002-pluglist.t -v
expecting success: 
	/home/garlick/proj/powerman/t/../src/powerman/test_pluglist -f p493 'foo[1-500]' 'p[1-500]' >find.out &&
	cat >find.exp <<-EOT
	plug=p493 node=foo493
	EOT

ok 1 - pluglist foo[1-500] p[1-500] maps expected host to p493

expecting success: 
	/home/garlick/proj/powerman/t/../src/powerman/test_pluglist 't[1,2,3]' 'p[999-1001]' >map1.out &&
	cat >map1.exp <<-EOT &&
	plug=p1001 node=t3
	plug=p1000 node=t2
	plug=p999 node=t1
	EOT
	test_cmp map1.exp map1.out

--- map1.exp	2026-05-28 14:00:03.993386336 +0000
+++ map1.out	2026-05-28 14:00:03.992179170 +0000
@@ -1,3 +1,3 @@
-plug=p1001 node=t3
-plug=p1000 node=t2
-plug=p999 node=t1
+plug=p00001001 node=t3
+plug=p00001000 node=t2
+plug=p00000999 node=t1
[many more failures]

Ah, this seems to be required on top of your change

@@ -1423,8 +1423,8 @@ static int _parse_single_range(const char *str, struct _range *range)
         seterrno_ret(ERANGE, 0);
     }
 
+    range->width = strlen(orig);
     free(orig);
-    range->width = strlen(str);
     return 1;
 
   error:

@garlick

garlick commented May 28, 2026

Copy link
Copy Markdown
Member

The same issue might exist in pdsh so ping @grondo

@garlick

garlick commented May 29, 2026

Copy link
Copy Markdown
Member

@hector-cao - let me know if you have time to amend and force push this commit with the proposed addition. If not I can submit a new PR. Thanks.

_parse_single_range() accepts a `const char *str` argument and creates
a mutable copy via strdup() into `orig`. However, it was incorrectly
calling strchr() and strtoul() on the original const pointer `str`
rather than on the mutable copy.

This is both a correctness bug and a build failure with modern glibc:
- glibc now provides const-preserving overloads of strchr(), returning
  `const char *` when passed a `const char *`. Assigning this to
  `char *p` discards the const qualifier, triggering a compile error
  with -Werror=discarded-qualifiers.
- The subsequent `*p++ = '\0'` write through `p` would modify memory
  via a pointer originally derived from a const string.

Fix by using `orig` (the mutable strdup copy) for strchr() and
strtoul() calls, which is the correct buffer to mutate.
@hector-cao

Copy link
Copy Markdown
Contributor Author

@hector-cao - let me know if you have time to amend and force push this commit with the proposed addition. If not I can submit a new PR. Thanks.

thanks for this speedy feedback, updated

@garlick

garlick commented May 29, 2026

Copy link
Copy Markdown
Member

Looking good here so let's merge. Thank you for running this down and for your excellent commit message! Let's merge.

@garlick garlick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@garlick
garlick merged commit fd3e965 into chaos:master May 29, 2026
7 of 8 checks passed
erentar added a commit to erentar/diod that referenced this pull request Jun 23, 2026
This fix was authored by hector-cao originally for chaos/powerman#216.

_parse_single_range() accepts a `const char *str` argument and creates
a mutable copy via strdup() into `orig`. However, it was incorrectly
calling strchr() and strtoul() on the original const pointer `str`
rather than on the mutable copy.

This is both a correctness bug and a build failure with modern glibc:
- glibc now provides const-preserving overloads of strchr(), returning
  `const char *` when passed a `const char *`. Assigning this to
  `char *p` discards the const qualifier, triggering a compile error
  with -Werror=discarded-qualifiers.
- The subsequent `*p++ = '\0'` write through `p` would modify memory
  via a pointer originally derived from a const string.

Fix by using `orig` (the mutable strdup copy) for strchr() and
strtoul() calls, which is the correct buffer to mutate.
erentar added a commit to erentar/diod that referenced this pull request Jun 23, 2026
This fix was authored by hector-cao originally for chaos/powerman#216.

_parse_single_range() accepts a `const char *str` argument and creates
a mutable copy via strdup() into `orig`. However, it was incorrectly
calling strchr() and strtoul() on the original const pointer `str`
rather than on the mutable copy.

This is both a correctness bug and a build failure with modern glibc:
- glibc now provides const-preserving overloads of strchr(), returning
  `const char *` when passed a `const char *`. Assigning this to
  `char *p` discards the const qualifier, triggering a compile error
  with -Werror=discarded-qualifiers.
- The subsequent `*p++ = '\0'` write through `p` would modify memory
  via a pointer originally derived from a const string.

Fix by using `orig` (the mutable strdup copy) for strchr() and
strtoul() calls, which is the correct buffer to mutate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants