Control character check #687
Conversation
|
You have successfully added a new shellcheck configuration |
| if (0 == err) { | ||
| /* Search if there are some non-printable characters */ | ||
| /* Search if there are some non-printable characters or control characters */ | ||
| for (cp = field; '\0' != *cp; cp++) { |
There was a problem hiding this comment.
Thank you - though please,
- squash the commits into one.
- just do "if (!isprint (*cp) || !iscntrl(*cp))
- 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)
There was a problem hiding this comment.
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! :)
There was a problem hiding this comment.
Sorry - you're right, my proposal is wrong. We should not switch to returning -1 instead of 1 for non-printables.
There was a problem hiding this comment.
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 There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Added control character check, returning -1 (to "err") if control characters are present.
|
@hallyn what do we do with the failing test case? |
I'll have to look into it this hopefully tonight, else this weekend. I'll look at the others you mentioned too. |
|
Oh - I see, it fails in your doc-only pr too. Let's just merge then. |
Added a check to search for control characters and return -1 (in "err") if found.