Skip to content

Control character check #687

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.

Sign up for GitHub

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

Merged
hallyn merged 1 commit into from
Mar 31, 2023
+7 −4
Merged

Control character check #687

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions lib/fields.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
*
* The supplied field is scanned for non-printable and other illegal
* characters.
* + -1 is returned if an illegal character is present.
* + 1 is returned if no illegal characters are present, but the field
* contains a non-printable character.
* + -1 is returned if an illegal or control character is present.
* + 1 is returned if no illegal or control characters are present,
* but the field contains a non-printable character.
* + 0 is returned otherwise.
*/
int valid_field (const char *field, const char *illegal)
Expand All @@ -45,10 +45,13 @@ int valid_field (const char *field, const char *illegal)
}

if (0 == err) {
/* Search if there are some non-printable characters */
/* Search if there are non-printable or control characters */
for (cp = field; '\0' != *cp; cp++) {
Copy link
Copy Markdown
Member

@hallyn hallyn Mar 27, 2023

Choose a reason for hiding this comment

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

Thank you - though please,

  1. squash the commits into one.
  2. just do "if (!isprint (*cp) || !iscntrl(*cp))
  3. still break when you set err = -1

so basically

for (cp = field; '\0' != *cp; cp++) {
    if (!isprint(*cp) || !iscntrl(*cp)) {
        err = -1;
        break;
    }
}

Thanks again! (and please let me know if you prefer I do this inline - I don't want it to get needlessly tedious for you)

Copy link
Copy Markdown
Contributor Author

@tomspiderlabs tomspiderlabs Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No problem. :) I did it this way because I didn’t think you wanted to ban non-printables also because that will break support for a lot of special characters in names? Your change makes this -1 and therefore non-printables will no longer be supported whereas currently this is permitted and returns 1. Can you just clarify you want to ban both? Banning control characters should be sufficient to stop the poc discussed, without impacting use cases still. I've made the change but let me know if this wasn't intended! :)

Copy link
Copy Markdown
Member

@hallyn hallyn Mar 28, 2023

Choose a reason for hiding this comment

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

Sorry - you're right, my proposal is wrong. We should not switch to returning -1 instead of 1 for non-printables.

Copy link
Copy Markdown
Contributor Author

@tomspiderlabs tomspiderlabs Mar 28, 2023

Choose a reason for hiding this comment

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

Updated. :)

Copy link
Copy Markdown
Member

@hallyn hallyn Mar 29, 2023

Choose a reason for hiding this comment

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

Thanks :)

Copy link
Copy Markdown
Collaborator

@alejandro-colomar alejandro-colomar Dec 11, 2024

Choose a reason for hiding this comment

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

Hi,

Let me re-open this old thread. I was revising the source code in this file, and the git-blame(1) led me to this patch, and the patch to this PR.

This patch doesn't look good (even ignoring the bug that was accidentally introduced, which was later fixed by @cgzones ): All ASCII characters are either isprint or iscntrl, but not both. See below.

We should revert this patch, for clarity. I'm preparing a round of patches.

$ cat ctype.c 
#include <ctype.h>
#include <limits.h>
#include <stdio.h>

int
main(void)
{
	printf("		AnApC D G L PrPuS U XdAsB\n");

	for (unsigned int i = 0; i <= UCHAR_MAX; i++) {
		printf("%d		",  i);
		printf(isalnum(i) ? "An" : "  ");
		printf(isalpha(i) ? "Ap" : "  ");
		printf(iscntrl(i) ? "C " : "  ");
		printf(isdigit(i) ? "D " : "  ");
		printf(isgraph(i) ? "G " : "  ");
		printf(islower(i) ? "L " : "  ");
		printf(isprint(i) ? "Pr" : "  ");
		printf(ispunct(i) ? "Pu" : "  ");
		printf(isspace(i) ? "S " : "  ");
		printf(isupper(i) ? "U " : "  ");
		printf(isxdigit(i) ? "Xd" : "  ");
		printf(isascii(i) ? "As" : "  ");
		printf(isblank(i) ? "B" : " ");
		puts("");
	}
}
$ ./a.out 
		AnApC D G L PrPuS U XdAsB
0		    C                 As 
1		    C                 As 
2		    C                 As 
3		    C                 As 
4		    C                 As 
5		    C                 As 
6		    C                 As 
7		    C                 As 
8		    C                 As 
9		    C           S     AsB
10		    C           S     As 
11		    C           S     As 
12		    C           S     As 
13		    C           S     As 
14		    C                 As 
15		    C                 As 
16		    C                 As 
17		    C                 As 
18		    C                 As 
19		    C                 As 
20		    C                 As 
21		    C                 As 
22		    C                 As 
23		    C                 As 
24		    C                 As 
25		    C                 As 
26		    C                 As 
27		    C                 As 
28		    C                 As 
29		    C                 As 
30		    C                 As 
31		    C                 As 
32		            Pr  S     AsB
33		        G   PrPu      As 
34		        G   PrPu      As 
35		        G   PrPu      As 
36		        G   PrPu      As 
37		        G   PrPu      As 
38		        G   PrPu      As 
39		        G   PrPu      As 
40		        G   PrPu      As 
41		        G   PrPu      As 
42		        G   PrPu      As 
43		        G   PrPu      As 
44		        G   PrPu      As 
45		        G   PrPu      As 
46		        G   PrPu      As 
47		        G   PrPu      As 
48		An    D G   Pr      XdAs 
49		An    D G   Pr      XdAs 
50		An    D G   Pr      XdAs 
51		An    D G   Pr      XdAs 
52		An    D G   Pr      XdAs 
53		An    D G   Pr      XdAs 
54		An    D G   Pr      XdAs 
55		An    D G   Pr      XdAs 
56		An    D G   Pr      XdAs 
57		An    D G   Pr      XdAs 
58		        G   PrPu      As 
59		        G   PrPu      As 
60		        G   PrPu      As 
61		        G   PrPu      As 
62		        G   PrPu      As 
63		        G   PrPu      As 
64		        G   PrPu      As 
65		AnAp    G   Pr    U XdAs 
66		AnAp    G   Pr    U XdAs 
67		AnAp    G   Pr    U XdAs 
68		AnAp    G   Pr    U XdAs 
69		AnAp    G   Pr    U XdAs 
70		AnAp    G   Pr    U XdAs 
71		AnAp    G   Pr    U   As 
72		AnAp    G   Pr    U   As 
73		AnAp    G   Pr    U   As 
74		AnAp    G   Pr    U   As 
75		AnAp    G   Pr    U   As 
76		AnAp    G   Pr    U   As 
77		AnAp    G   Pr    U   As 
78		AnAp    G   Pr    U   As 
79		AnAp    G   Pr    U   As 
80		AnAp    G   Pr    U   As 
81		AnAp    G   Pr    U   As 
82		AnAp    G   Pr    U   As 
83		AnAp    G   Pr    U   As 
84		AnAp    G   Pr    U   As 
85		AnAp    G   Pr    U   As 
86		AnAp    G   Pr    U   As 
87		AnAp    G   Pr    U   As 
88		AnAp    G   Pr    U   As 
89		AnAp    G   Pr    U   As 
90		AnAp    G   Pr    U   As 
91		        G   PrPu      As 
92		        G   PrPu      As 
93		        G   PrPu      As 
94		        G   PrPu      As 
95		        G   PrPu      As 
96		        G   PrPu      As 
97		AnAp    G L Pr      XdAs 
98		AnAp    G L Pr      XdAs 
99		AnAp    G L Pr      XdAs 
100		AnAp    G L Pr      XdAs 
101		AnAp    G L Pr      XdAs 
102		AnAp    G L Pr      XdAs 
103		AnAp    G L Pr        As 
104		AnAp    G L Pr        As 
105		AnAp    G L Pr        As 
106		AnAp    G L Pr        As 
107		AnAp    G L Pr        As 
108		AnAp    G L Pr        As 
109		AnAp    G L Pr        As 
110		AnAp    G L Pr        As 
111		AnAp    G L Pr        As 
112		AnAp    G L Pr        As 
113		AnAp    G L Pr        As 
114		AnAp    G L Pr        As 
115		AnAp    G L Pr        As 
116		AnAp    G L Pr        As 
117		AnAp    G L Pr        As 
118		AnAp    G L Pr        As 
119		AnAp    G L Pr        As 
120		AnAp    G L Pr        As 
121		AnAp    G L Pr        As 
122		AnAp    G L Pr        As 
123		        G   PrPu      As 
124		        G   PrPu      As 
125		        G   PrPu      As 
126		        G   PrPu      As 
127		    C                 As 
128		                         
129		                         
130		                         
131		                         
132		                         
133		                         
134		                         
135		                         
136		                         
137		                         
138		                         
139		                         
140		                         
141		                         
142		                         
143		                         
144		                         
145		                         
146		                         
147		                         
148		                         
149		                         
150		                         
151		                         
152		                         
153		                         
154		                         
155		                         
156		                         
157		                         
158		                         
159		                         
160		                         
161		                         
162		                         
163		                         
164		                         
165		                         
166		                         
167		                         
168		                         
169		                         
170		                         
171		                         
172		                         
173		                         
174		                         
175		                         
176		                         
177		                         
178		                         
179		                         
180		                         
181		                         
182		                         
183		                         
184		                         
185		                         
186		                         
187		                         
188		                         
189		                         
190		                         
191		                         
192		                         
193		                         
194		                         
195		                         
196		                         
197		                         
198		                         
199		                         
200		                         
201		                         
202		                         
203		                         
204		                         
205		                         
206		                         
207		                         
208		                         
209		                         
210		                         
211		                         
212		                         
213		                         
214		                         
215		                         
216		                         
217		                         
218		                         
219		                         
220		                         
221		                         
222		                         
223		                         
224		                         
225		                         
226		                         
227		                         
228		                         
229		                         
230		                         
231		                         
232		                         
233		                         
234		                         
235		                         
236		                         
237		                         
238		                         
239		                         
240		                         
241		                         
242		                         
243		                         
244		                         
245		                         
246		                         
247		                         
248		                         
249		                         
250		                         
251		                         
252		                         
253		                         
254		                         
255

Copy link
Copy Markdown
Collaborator

@alejandro-colomar alejandro-colomar Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On the other hand, the return value of 1 was completely being ignored in most calls, checking explicitly against -1, so we're better with this -1.

I'll reform this function to only report -1 or 0.

In fact, we're already only returning 0 or -1. 1 can never happen.

Copy link
Copy Markdown
Collaborator

@alejandro-colomar alejandro-colomar Dec 11, 2024

Choose a reason for hiding this comment

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

Although, multi-byte sequences did actually return 1, so maybe we do want to keep allowing them by returning 1 for them. Since this function is called on the gecos field, that would make sense.

In that case, the current code is good, but confusing. I'll see what I can do.

if (!isprint (*cp)) {
err = 1;
}
if (!iscntrl (*cp)) {
err = -1;
break;
}
}
Expand Down