simplify ifconfig's len_string and print_string

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

simplify ifconfig's len_string and print_string

Stefan Sperling-5
These functions will compute the wrong display width for input which is
already a hex string. This bug doesn't trigger because we never actually
pass an ASCII hex string in, but it is still a bug.

ASCII hex strings are printable ASCII, so there's no reason to have a
special case for them.

This special case for "0x" was added in r1.60 of ifconfig.c ("Add support for
nwkey and powersave; from NetBSD") by millert. At that time, the print_string
function did not compute a length, and ifconfig was still using print_string
to print WEP keys to stdout, which seems to be why the check for "0x" existed.
WEP keys could be either printable ASCII or ASCII hex.

Later on, phessler and I copied the same check to len_string which is based
on print_string.

We don't print keys nowadays, and these functions are used only to print
binary data SSIDs (ASCII hex is an output, never an input).

So we can simplify this.

ok?

diff c20bd74017ceeadb2db0f78a352ed1f1e2b77c2b /usr/src
blob - e1dc9dbb07bf109c3ec7f5fd4d851a7dbb5692f1
file + sbin/ifconfig/ifconfig.c
--- sbin/ifconfig/ifconfig.c
+++ sbin/ifconfig/ifconfig.c
@@ -1681,16 +1681,13 @@ get_string(const char *val, const char *sep, u_int8_t
 int
 len_string(const u_int8_t *buf, int len)
 {
- int i = 0, hasspc = 0;
+ int i, hasspc = 0;
 
- if (len < 2 || buf[0] != '0' || tolower(buf[1]) != 'x') {
- for (; i < len; i++) {
- /* Only print 7-bit ASCII keys */
- if (buf[i] & 0x80 || !isprint(buf[i]))
- break;
- if (isspace(buf[i]))
- hasspc++;
- }
+ for (i = 0; i < len; i++) {
+ if (buf[i] & 0x80 || !isprint(buf[i]))
+ break;
+ if (isspace(buf[i]))
+ hasspc++;
  }
  if (i == len) {
  if (hasspc || len == 0)
@@ -1704,16 +1701,14 @@ len_string(const u_int8_t *buf, int len)
 int
 print_string(const u_int8_t *buf, int len)
 {
- int i = 0, hasspc = 0;
+ int i, hasspc = 0;
 
- if (len < 2 || buf[0] != '0' || tolower(buf[1]) != 'x') {
- for (; i < len; i++) {
- /* Only print 7-bit ASCII keys */
- if (buf[i] & 0x80 || !isprint(buf[i]))
- break;
- if (isspace(buf[i]))
- hasspc++;
- }
+ for (i = 0; i < len; i++) {
+ /* Only print 7-bit ASCII printable characters. */
+ if (buf[i] & 0x80 || !isprint(buf[i]))
+ break;
+ if (isspace(buf[i]))
+ hasspc++;
  }
  if (i == len) {
  if (hasspc || len == 0) {

Reply | Threaded
Open this post in threaded view
|

Re: simplify ifconfig's len_string and print_string

Stuart Henderson
On 2020/02/24 11:56, Stefan Sperling wrote:

> These functions will compute the wrong display width for input which is
> already a hex string. This bug doesn't trigger because we never actually
> pass an ASCII hex string in, but it is still a bug.
>
> ASCII hex strings are printable ASCII, so there's no reason to have a
> special case for them.
>
> This special case for "0x" was added in r1.60 of ifconfig.c ("Add support for
> nwkey and powersave; from NetBSD") by millert. At that time, the print_string
> function did not compute a length, and ifconfig was still using print_string
> to print WEP keys to stdout, which seems to be why the check for "0x" existed.
> WEP keys could be either printable ASCII or ASCII hex.
>
> Later on, phessler and I copied the same check to len_string which is based
> on print_string.
>
> We don't print keys nowadays, and these functions are used only to print
> binary data SSIDs (ASCII hex is an output, never an input).
>
> So we can simplify this.
>
> ok?

We do still need the 0x special case otherwise the user won't be able
to figure out how to connect to networks with certain names without
referring to an ascii-hex table.

Reply | Threaded
Open this post in threaded view
|

Re: simplify ifconfig's len_string and print_string

Stefan Sperling-5
On Tue, Feb 25, 2020 at 10:40:47AM +0000, Stuart Henderson wrote:

> On 2020/02/24 11:56, Stefan Sperling wrote:
> > These functions will compute the wrong display width for input which is
> > already a hex string. This bug doesn't trigger because we never actually
> > pass an ASCII hex string in, but it is still a bug.
> >
> > ASCII hex strings are printable ASCII, so there's no reason to have a
> > special case for them.
> >
> > This special case for "0x" was added in r1.60 of ifconfig.c ("Add support for
> > nwkey and powersave; from NetBSD") by millert. At that time, the print_string
> > function did not compute a length, and ifconfig was still using print_string
> > to print WEP keys to stdout, which seems to be why the check for "0x" existed.
> > WEP keys could be either printable ASCII or ASCII hex.
> >
> > Later on, phessler and I copied the same check to len_string which is based
> > on print_string.
> >
> > We don't print keys nowadays, and these functions are used only to print
> > binary data SSIDs (ASCII hex is an output, never an input).
> >
> > So we can simplify this.
> >
> > ok?
>
> We do still need the 0x special case otherwise the user won't be able
> to figure out how to connect to networks with certain names without
> referring to an ascii-hex table.

I'm not sure I understand what you mean. Can you give an example?

If we need to keep this then don't we still need to fix the return value?

Reply | Threaded
Open this post in threaded view
|

Re: simplify ifconfig's len_string and print_string

Stuart Henderson
On 2020/02/25 13:00, Stefan Sperling wrote:

> On Tue, Feb 25, 2020 at 10:40:47AM +0000, Stuart Henderson wrote:
> > On 2020/02/24 11:56, Stefan Sperling wrote:
> > > These functions will compute the wrong display width for input which is
> > > already a hex string. This bug doesn't trigger because we never actually
> > > pass an ASCII hex string in, but it is still a bug.
> > >
> > > ASCII hex strings are printable ASCII, so there's no reason to have a
> > > special case for them.
> > >
> > > This special case for "0x" was added in r1.60 of ifconfig.c ("Add support for
> > > nwkey and powersave; from NetBSD") by millert. At that time, the print_string
> > > function did not compute a length, and ifconfig was still using print_string
> > > to print WEP keys to stdout, which seems to be why the check for "0x" existed.
> > > WEP keys could be either printable ASCII or ASCII hex.
> > >
> > > Later on, phessler and I copied the same check to len_string which is based
> > > on print_string.
> > >
> > > We don't print keys nowadays, and these functions are used only to print
> > > binary data SSIDs (ASCII hex is an output, never an input).
> > >
> > > So we can simplify this.
> > >
> > > ok?
> >
> > We do still need the 0x special case otherwise the user won't be able
> > to figure out how to connect to networks with certain names without
> > referring to an ascii-hex table.
>
> I'm not sure I understand what you mean. Can you give an example?
>
> If we need to keep this then don't we still need to fix the return value?
>

Say you have an access point broadcasting these SSIDs (literally):

- 0xfoobar

without the 0x case in print_string it will be printed as 0xfoobar but
you need to use 0x666f6f626172 to connect.

- 0x6162616374c3a97269c3a96d69717565
- abactériémique

without the 0x case, they will both appear the same in the list.
with the 0x case they will both be encoded so can be distinguished (and
so you can copy-and-paste to ifconfig nwid).