-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace fgetsx() by fgets(3) and fputsx() by fputs(3) #1056
base: master
Are you sure you want to change the base?
Conversation
lib/gshadow.c
Outdated
if (fgetsx(buf, buflen, fp) == buf) { | ||
while ( ((cp = strrchr (buf, '\n')) == NULL) | ||
&& (feof (fp) == 0)) { | ||
size_t len; | ||
|
||
cp = REALLOC(buf, buflen * 2, char); | ||
if (NULL == cp) { | ||
return NULL; | ||
} | ||
buf = cp; | ||
buflen *= 2; | ||
|
||
len = strlen (buf); | ||
if (fgetsx (&buf[len], | ||
(int) (buflen - len), | ||
fp) != &buf[len]) { | ||
return NULL; | ||
} | ||
if (fgets(buf, buflen, fp) == NULL) | ||
return NULL; | ||
|
||
while ( (strrchr(buf, '\n') == NULL) | ||
&& (feof (fp) == 0)) { | ||
size_t len; | ||
|
||
cp = REALLOC(buf, buflen * 2, char); | ||
if (NULL == cp) { | ||
return NULL; | ||
} | ||
stpsep(buf, "\n"); | ||
return (sgetsgent (buf)); | ||
buf = cp; | ||
buflen *= 2; | ||
|
||
len = strlen (buf); | ||
if (fgets(&buf[len], buflen - len, fp) == NULL) | ||
return NULL; | ||
} | ||
return NULL; | ||
stpsep(buf, "\n"); | ||
return (sgetsgent (buf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sam,
It might be worth commenting this here.
Here's an overview of the branch:
$ git log --oneline master..fgets
cd64d530 (HEAD -> fgets, gh/fgets) lib/, po/: Remove fgetsx() and fputsx()
941c951c lib/, src/: Remove useless casts in fgets(3)
4d66df60 lib/, src/: Consistently use NULL with fgets(3)
d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
a719bc39 (gh/string, string) lib/fields.c: Remove dead code
b71d2236 contrib/, lib/, src/: Use consistent style using strchr(3) in conditionals
44df1083 lib/, src/: Use str[n]cmp(3) instead of explicit byte comparisons
d3350b67 contrib/, lib/, src/: Use strchr(3) to compact comparisons
9a6e3284 lib/loginprompt.c: login_prompt(): Use strtcpy() instead of its pattern
db93a222 lib/, src/: Use strsep(3) instead of strtok(3)
376e4166 src/suauth.c: check_su_auth(): Use pointers to simplify
ae1c284e src/suauth.c: check_su_auth(): Use strspn(3) instead of its pattern
93362cc3 lib/gshadow.c: endsgent(): Remove dead assignment
5a46723a lib/port.c: portcmp(): Use strcmp(3) instead of its pattern
7442a127 lib/, src/: Use stpspn() instead of its pattern
This fgets
branch is really only
$ git log --oneline string..fgets
cd64d530 (HEAD -> fgets, gh/fgets) lib/, po/: Remove fgetsx() and fputsx()
941c951c lib/, src/: Remove useless casts in fgets(3)
4d66df60 lib/, src/: Consistently use NULL with fgets(3)
d0cdc9c8 lib/gshadow.c: endsgent(): Invert logic to reduce indentation
but I based it on string
, which corresponds to PR #1048.
Here's why. Basically, according to the bug report that it closes, the PR is about replacing fgetsx => fgets. Here are two of the calls that we replace:
$ grep -rnC4 fgetsx lib/gshadow.c
154- if (NULL == fp) {
155- return NULL;
156- }
157-
158: if (fgetsx(buf, buflen, fp) == buf) {
159- while ( ((cp = strrchr (buf, '\n')) == NULL)
160- && (feof (fp) == 0)) {
161- size_t len;
162-
--
167- buf = cp;
168- buflen *= 2;
169-
170- len = strlen (buf);
171: if (fgetsx (&buf[len],
172- (int) (buflen - len),
173- fp) != &buf[len]) {
174- return NULL;
175- }
I want to fully understand what the code is doing before doing the change, which makes me more confident that the change is correct. To that end, I need to reduce superfluous complexity.
In the code above, you may notice a dead assignment:
159- while ( ((cp = strrchr (buf, '\n')) == NULL)
I already had removed that dead assignment in #1048 in commit 93362cc. That's the only reason why I decided to base it on that PR.
Now let's see why this PR has 4 new commits, instead of 1.
Line
158: if (fgetsx(buf, buflen, fp) == buf) {
Has a conditional that spans the entire rest of the function. Commit 1 turns this into the usual thing, which is an early return on error.
Then we get to
171: if (fgetsx (&buf[len],
172- (int) (buflen - len),
173- fp) != &buf[len]) {
Why do we have that cast? It's nonsense. I'll remove it.
And also &buf[len]
is very brittle, and a small typo could add a bug here. Let's use NULL there.
While doing those two changes, I'll check any other places that do similarly bad stuff, and replace it in the entire code base. Then commit 4 does what we originally wanted.
I don't reason too much the first 3 commits, which you might find frustrating while reviewing the changes a posteriori. I didn't because they're not the main purpose of the PR; they're just trivial cosmetic changes in preparation for the PR. And it's usual to apply cosmetic patches as the first patches in a patch set, to prepare for the important ones.
Do you have any suggestions for making it easier for you to review the changes later?
Maybe I could add an explicit [cosmetic]
in the second line of the commit message, to mark those as unimportant commits.
Still, I need to apply those cosmetic changes, or it would be much more difficult to me to reason about the important changes of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And these cosmetic patches combined with the change fgetsx=>fgets have allowed me to see a transformation fgets=>getline too. I probably wouldn't have been able to see it without the cosmetic changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider my coccinelle suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how to use it. If you point me to some docs and/or show me a small shell example session showing how it's useful (the example will help more than the docs, TBH), I'll be happy to try it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. https://coccinelle.gitlabpages.inria.fr/website/sp.html. Sounds like it could be interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I mentioned it before ;)
I think it's both a good fit for the work you're interested in and eases review of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to develop a coccinelle script that would do something similar to 941c951
So far, I've written
$ cat fgets_cast.sp
@@
expression a, b, c;
type t;
@@
- fgets(a, (t) (b), c)
+ fgets(a, b, c)
@@
expression a, b, c;
type t;
@@
- fgets(a, (t) b, c)
+ fgets(a, b, c)
@@
expression a, b, c;
type t;
@@
- fgetsx(a, (t) (b), c)
+ fgetsx(a, b, c)
@@
expression a, b, c;
type t;
@@
- fgetsx(a, (t) b, c)
+ fgetsx(a, b, c)
@@
expression a, b, c, p;
type t;
@@
- p->fgets(a, (t) (b), c)
+ p->fgets(a, b, c)
@@
expression a, b, c, p;
type t;
@@
- p->fgets(a, (t) b, c)
+ p->fgets(a, b, c)
and I'm running it as
$ find lib* src/ -type f \
| xargs spatch --sp-file ~/tmp/spatch/fgets_cast.sp --in-place;
but it results in false positives, such as
diff --git i/lib/shadow.c w/lib/shadow.c
index 44436836..5759f259 100644
--- i/lib/shadow.c
+++ w/lib/shadow.c
@@ -202,7 +202,7 @@ struct spwd *fgetspent (FILE * fp)
return (0);
}
- if (fgets (buf, sizeof buf, fp) != NULL)
+ if (fgets(buf, sizeof buf, fp) != NULL)
{
stpsep(buf, "\n");
return my_sgetspent (buf);
How is that matching (t)
, if there are no casts?
@@ -81,9 +84,8 @@ void login_prompt (char *name, int namesize) | |||
*/ | |||
|
|||
MEMZERO(buf); | |||
if (fgets (buf, sizeof buf, stdin) != buf) { | |||
if (fgets(buf, sizeof(buf), stdin) == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful if formatting changes weren't mixed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to keep those to a minimum: only whitespace and (sometimes) parentheses. And only on hunks where it (subjectively, admittedly) doesn't hurt too much readability (i.e., if I'm doing some huge transformations where I need your attention somewhere, I'll try to not do it there.
I'll keep in mind your concern.
/* | ||
* fgetsgent - convert next line in stream to (struct sgrp) | ||
* | ||
* fgetsgent() reads the next line from the provided stream and | ||
* converts it to a (struct sgrp). NULL is returned on EOF. | ||
*/ | ||
|
||
/*@observer@*//*@null@*/struct sgrp *fgetsgent (/*@null@*/FILE * fp) | ||
/*@observer@*//*@null@*/struct sgrp * | ||
fgetsgent(/*@null@*/FILE *fp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this null stuff really needed? Can't we use the attribute for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some patches for reducing those comments some time ago, and replaced some of them by attributes and qualifiers
I should have another look at those at some point, and send another round of patches.
@@ -66,7 +68,8 @@ int valid_field (const char *field, const char *illegal) | |||
* prompt the user with the name of the field being changed and the | |||
* current value. | |||
*/ | |||
void change_field (char *buf, size_t maxsize, const char *prompt) | |||
void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really, if you must do this, can't you just do a huge clang-format pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'll have a look at using it. Thanks for the suggestion.
I really feel this PR is still pretty noisy. |
0936efa
to
5100579
Compare
bzero (cmd, sizeof (cmd)); | ||
bzero(cmd, sizeof(cmd)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you apply the semantic patch accompanied with the corresponding commit message, and then git diff -w
against the actual commit, you should be able to ignore these unrelated whitespace changes. I've written recommendations for how to better review each commit.
I hope that helps reviewing, while at the same time allowing me to apply some house style while doing the changes.
a71fc6f
to
e49bcf8
Compare
f7d5cc1
to
ef91817
Compare
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
This is simpler to read, IMO. Signed-off-by: Alejandro Colomar <[email protected]>
…onals For negative matches, use if (strchr(...) == NULL) For positive matches, use the cast-to-bool operator: if (!!strchr(...)) For positive matches, when a variable is also set, use while (NULL != (p = strchr(...))) Signed-off-by: Alejandro Colomar <[email protected]>
A few lines above, we've removed the '\n' already. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
sizeof(foo) - No spaces. Not: sizeof (foo) - Parentheses. Not: sizeof foo - No parentheses wrapping sizeof itself Not: (sizeof foo) This patch can be approximated with the following semantic patch: $ cat ~/tmp/spatch/sizeof.sp @@ identifier a, b; @@ - sizeof a->b + sizeof(a->b) @@ identifier a, b; @@ - sizeof a.b + sizeof(a.b) @@ identifier x; @@ - sizeof x + sizeof(x) @@ identifier x; @@ - sizeof *x + sizeof(*x) @@ identifier x; @@ - (sizeof(x)) + sizeof(x) @@ identifier x; @@ - (sizeof(*x)) + sizeof(*x) Applied as $ find contrib/ lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/sizeof.sp --in-place; The differences between the actual patch and the approximation via the semantic patch from above are whitespace only. When reviewing, it'll be useful to diff with '-w'. Link: <https://lkml.org/lkml/2012/7/11/103> Signed-off-by: Alejandro Colomar <[email protected]>
This patch can be replicated with the following semantic patch: $ cat fgets_cast.sp @@ expression a, b, c; @@ - fgets(a, (int) (b), c) + fgets(a, b, c) @@ expression a, b, c; @@ - fgets(a, (int) b, c) + fgets(a, b, c) @@ expression a, b, c; @@ - fgetsx(a, (int) (b), c) + fgetsx(a, b, c) @@ expression a, b, c; @@ - fgetsx(a, (int) b, c) + fgetsx(a, b, c) @@ expression a, b, c, p; @@ - p->fgets(a, (int) (b), c) + p->fgets(a, b, c) @@ expression a, b, c, p; @@ - p->fgets(a, (int) b, c) + p->fgets(a, b, c) which is applied as: $ find lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/fgets_cast.sp --in-place; Signed-off-by: Alejandro Colomar <[email protected]>
fgets(3) returns either NULL or the input pointer. Checking for NULL is more explicit, and simpler. <stddef.h> is the header that provides NULL; add it where appropriate. The meat of this patch can be approximated with the following semantic patch: $ cat ~/tmp/spatch/fgets_null.sp @@ expression a, b, c; @@ - fgets(a, b, c) == a + fgets(a, b, c) != NULL @@ expression a, b, c; @@ - fgetsx(a, b, c) == a + fgetsx(a, b, c) != NULL @@ expression a, b, c, p; @@ - p->fgets(a, b, c) == a + p->fgets(a, b, c) != NULL @@ expression a, b, c; @@ - fgets(a, b, c) != a + fgets(a, b, c) == NULL @@ expression a, b, c; @@ - fgetsx(a, b, c) != a + fgetsx(a, b, c) == NULL @@ expression a, b, c, p; @@ - p->fgets(a, b, c) != a + p->fgets(a, b, c) == NUL Applied as $ find contrib/ lib* src/ -type f \ | xargs spatch --sp-file ~/tmp/spatch/fgets_null.sp --in-place; The differences between the actual patch and the approximation via the semantic patch from above are includes, whitespace, braces, and a case where there was an implicit pointer-to-bool comparison which I made explicit. When reviewing, it'll be useful to use git-diff(1) with '-w' and '--color-words=.'. Signed-off-by: Alejandro Colomar <[email protected]>
It seems they never worked correctly. Let's keep it simple and remove support for escaped newlines. Closes: <shadow-maint#1055> Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
ef91817
to
27dd9f5
Compare
It seems they never worked correctly. Let's keep it simple and remove
support for escaped newlines.
Closes: #1055
Reported-by: @zeha
(Merge after #1048.)
Revisions:
v1b
v2
v2b
v3
v3b
v4
-w
and/or--color-words=.
, so that it doesn't hurt review.v4b
v4c
v4d
v4e
v4f