81585 - oddity in mailnews/news/src/nsNNTPProtocol.cpp

Status: RESOLVED INVALID

(MailNews Core :: Networking: NNTP, defect)

Description

[Ulrich Drepper]

Today I was looking through the mozilla code to find uses of the evil function
strtok().  While doing this I came across this strange code in

   mailnews/news/src/nsNNTPProtocol.cpp, line 4042

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                if(group)
                {
                        *group++ = '\0';
                        /* the group name may be contaminated by "group selected
" at
                           the end.  This will be space separated from the group
 name.
                           If a space is found in the group name terminate at th
at
                           point. */
                        strtok(group, " ");
                        last_art = atol(high);
                }
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The use of strtok() here does not fulfill any purpose.  The return value is not
used, the function is never called with a NULL value for the first parameter.

From the comment one can guess that the function is supposed to skip some white
space but beside not doing this at all there are certainly better ways.

I have no concrete proposals since I don't think I understand this strange code.
 But if this function works fine as it is the strtok() can simply be removed
without loss.

Comment 1

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Patch

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

I have no idea if what you're suggesting is correct or not (only Seth would
know), but there's a patch anyway.

Comment 3

[[e-mail] (gone - use [e-mail] instead)]

jag, Naving, any thoughts on this?

Comment 4

[jag (Peter Annema)]

man strtok

...

If a token ends with a delimiter, this delimiting character is overwritten with
a \0 and a pointer to the next charac­ter is saved for the next call to
strtok(). The delimiter string delim may be different for each call.

...

The true evilness of strtok is this hidden side effect of overwriting the space
with a '\0'. The effect of this code here is:

while (*group != '\0') {
  if (*group == ' ') {
    *group = '\0';
    break;
  }
  ++group;
}

Comment 5

[Ulrich Drepper]

> The true evilness of strtok is this hidden side effect of overwriting the
> space with a '\0'. The effect of this code here is:
>
> while (*group != '\0') {
>   if (*group == ' ') {
>     *group = '\0';
>     break;
>   }
>   ++group;
> }

That's not true.  What the code does is this:

if (group)
{
  *group++ = '\0';
  const char *tmp = group;
  while (*tmp == ' ')
    ++tmp;
  if (*tmp != ' ')
    while (*tmp != '\0')
      if (*tmp != ' ')
      {
        *tmp = '\0';
        break;
      }
      else
        ++tmp;
}

I.e., if only spaces follow no NUL byte is added.

And beside this, strtok() is not thread-safe.  Simply replace the strtok() call
with something correct and obvious.

Comment 6

[jag (Peter Annema)]

If I've interpreted the comment right above it correctly, there are two start
scenarios: one where the following string is "group" and one where it is "group
selected", so I assumed I could skip the |while (*tmp == ' ') ++tmp;| step,
though I should have used a tmp var instead of modifying |group|.

Btw, you don't need to |if (*tmp != ' ')| since the while loop above that
guarantees it. And no need for |else| after |if (...) { ...; break; }|.

Comment 7

[Ulrich Drepper]

> Btw, you don't need to |if (*tmp != ' ')| since the while loop above that
> guarantees it.

This is a typo on my side.  This should be

if (*tmp != '\0')

> And no need for |else| after |if (...) { ...; break; }

Of course this is needed.  THis is simply how I write it *without* { } around
the while block.

Comment 8

[jag (Peter Annema)]

Duh. Indeed it is needed without those {} around the while block. I'm so used to
our preference ("coding-style") of having {} about blocks of more than one line
that I didn't even notice they were absent.

Btw, also no need to |if (*tmp != '\0')| since your |while (*tmp != '\0')| will
catch that.

Comment 9

[odi]

mailnews/news/src/nsNNTPProtocol.cpp should be reworked and broken-up into 
smaller pieces at some point anyway. It is huge, full of to-do tags. It seems 
that this code has grown a lot since start.

Comment 10

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 11

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 12

[Joshua Cranmer [:jcranmer]]

I don't object to the use of the strtok here.