oddity in mailnews/news/src/nsNNTPProtocol.cpp

RESOLVED INVALID

Status

MailNews Core
Networking: NNTP
--
minor
RESOLVED INVALID
17 years ago
9 years ago

People

(Reporter: Ulrich Drepper, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
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.
OS: Linux → All
QA Contact: esther → stephend
Hardware: PC → All
Created attachment 37212 [details] [diff] [review]
Patch
I have no idea if what you're suggesting is correct or not (only Seth would
know), but there's a patch anyway.
jag, Naving, any thoughts on this?

Comment 4

16 years ago
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;
}
(Reporter)

Comment 5

16 years ago
> 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

16 years ago
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; }|.
(Reporter)

Comment 7

16 years ago
> 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

16 years ago
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

16 years ago
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.

Updated

16 years ago
Severity: normal → minor
Product: MailNews → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: stephend → networking.news
(Assignee)

Updated

9 years ago
Product: Core → MailNews Core
I don't object to the use of the strtok here.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.