Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
On Fri, Mar 18, 2022 at 02:11:27PM +0600, NRK wrote:
> On Fri, Mar 18, 2022 at 12:40:00AM +0100, Roberto E. Vargas Caballero wrote:
> > Uh, I didn't realize about it, I just saw that having 255 entries was
> > wrong ^^!!!.
> > I think the best approach is to split the commit in two and evaluate/review
> > them isolated; one commit to fix the size, and other about zeroing.
>
> Digging a bit more into this, I see that the array is only being
> accessed in here, inside base64dec():
>
> while (*src) {
> int a = base64_digits[(unsigned char) base64dec_getc()];
> int b = base64_digits[(unsigned char) base64dec_getc()];
> int c = base64_digits[(unsigned char) base64dec_getc()];
> int d = base64_digits[(unsigned char) base64dec_getc()];
>
> This makes me think that the array should've just been local to the
> function, instead of being declared at file-scope. Anyhow, let's look at
> base64dec_getc():
>
> char
> base64dec_getc(const char **src)
> {
> while (**src && !isprint(**src))
> (*src)++;
> return **src ? *((*src)++) : '='; /* emulate padding if string
> ends */
> }
>
> Seems like it's only going to return either a printable char, or '='.
> Afaik the highest printable ascii char is 127, so maybe the size of the
> array can be reduced to just 128.
>
> Another thing that comes to mind is that fact that all the
> functions have bit of a bear-trap in them:
>
> In all cases the argument is an int, the value of which shall be
> representable as an unsigned char or shall equal the value of
> the macro EOF. If the argument has any other value, the behavior
> is undefined.
>
> So maybe **src should also be casted to an unsigned char before passing
> it to isprint().
>
> - NRK
>
I agree for isprint((unsigned char)).
But please make that a separate patch.
Thanks,
--
Kind regards,
Hiltjo
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
On Fri, Mar 18, 2022 at 12:40:00AM +0100, Roberto E. Vargas Caballero wrote:
> Uh, I didn't realize about it, I just saw that having 255 entries was
> wrong ^^!!!.
> I think the best approach is to split the commit in two and evaluate/review
> them isolated; one commit to fix the size, and other about zeroing.
Digging a bit more into this, I see that the array is only being
accessed in here, inside base64dec():
while (*src) {
int a = base64_digits[(unsigned char) base64dec_getc()];
int b = base64_digits[(unsigned char) base64dec_getc()];
int c = base64_digits[(unsigned char) base64dec_getc()];
int d = base64_digits[(unsigned char) base64dec_getc()];
This makes me think that the array should've just been local to the
function, instead of being declared at file-scope. Anyhow, let's look at
base64dec_getc():
char
base64dec_getc(const char **src)
{
while (**src && !isprint(**src))
(*src)++;
return **src ? *((*src)++) : '='; /* emulate padding if string
ends */
}
Seems like it's only going to return either a printable char, or '='.
Afaik the highest printable ascii char is 127, so maybe the size of the
array can be reduced to just 128.
Another thing that comes to mind is that fact that all the
functions have bit of a bear-trap in them:
In all cases the argument is an int, the value of which shall be
representable as an unsigned char or shall equal the value of
the macro EOF. If the argument has any other value, the behavior
is undefined.
So maybe **src should also be casted to an unsigned char before passing
it to isprint().
- NRK
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
Hi, On Fri, Mar 18, 2022 at 03:16:56AM +0600, NRK wrote: > But yes, you're right, you'd need 256 elements to be able to index into > an array as any unsigned char. So maybe it *should* be 256. Uh, I didn't realize about it, I just saw that having 255 entries was wrong ^^!!!. I think the best approach is to split the commit in two and evaluate/review them isolated; one commit to fix the size, and other about zeroing. Regards,
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
On Thu, Mar 17, 2022 at 09:44:35PM +0100, Roberto E. Vargas Caballero wrote: > I meant 256. If you want to be able to use any unsigned char as index > in the array then you need 256 positions, from 0 to 255. The patch > also had that wrong. > > Also, the comment of the patch only stayed that it was avoiding zeroing, > it didn't say anything about trying (but failing) increase the portability > of the code. It was a unneded modification for the target of the patch > (as it was described in the comment of the commit). Hi Roberto, The previous array had 255 element, not 256. The patch didn't change that, so all the patch does, effectively, is remove explicit zeroing; just as the commit message describes. But yes, you're right, you'd need 256 elements to be able to index into an array as any unsigned char. So maybe it *should* be 256. - NRK
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
On Thu, Mar 17, 2022 at 08:25:09PM +0100, Roberto E. Vargas Caballero wrote:
> On Tue, Mar 15, 2022 at 04:30:52PM +0600, NRK wrote:
> > +static const char base64_digits[(unsigned char)-1] = {
>
> Any reason to write "(unsigned char)-1" instead of writing 256?
Didn't hardcode 255 for pedantic reasons (since char is guranteed to be
at least, but not exactly 8bits), and didn't use UCHAR_MAX to avoid
needlessly including .
But given POSIX mandates 8bit chars, hardcoding 255 shouldn't be a
problem in practice.
- NRK
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
On Thu, 17 Mar 2022 20:25:09 +0100
"Roberto E. Vargas Caballero" wrote:
Dear Roberto,
> On Tue, Mar 15, 2022 at 04:30:52PM +0600, NRK wrote:
> > +static const char base64_digits[(unsigned char)-1] = {
>
> Any reason to write "(unsigned char)-1" instead of writing 256?
char is not guaranteed to be 8-Bit (unless we assume Posix, which is
reasonable within Posix), and you probably meant 255. An alternative
would be to go with UCHAR_MAX from limits.h.
With best regards
Laslo
Re: [hackers] [st][PATCH] rm unnecessary explicit zeroing
Hi,
On Tue, Mar 15, 2022 at 04:30:52PM +0600, NRK wrote:
> +static const char base64_digits[(unsigned char)-1] = {
Any reason to write "(unsigned char)-1" instead of writing 256?
Regards,
