Re: [9fans] manual suggestions and upas/fs bug

Re: [9fans] manual suggestions and upas/fs bug

[Russ Cox]

> p = realloc(p,n) is almost always a mistake.  If the allocation
> fails, the previous pointer value in p gets replaced by a null
> pointer value, so you lose the handle to the data that did not
> get extended (or, rarely, shrunk).

maybe this is what lucio was getting at, but when the next line is

	if(p == 0)
		sysfatal("out of memory");

then it doesn't really matter that we've lost the old p.

Re: [9fans] manual suggestions and upas/fs bug

[Russ Cox]

> We have both client and server bugs here.

Thanks for your pointers.  The IMAP client code has been
cobbled together by a few people (myself included) who knew
nothing about IMAP before working on it, so it's not surprising
that these inefficiencies exist.

> And *again* the server said there were no messages, thus
>
>    -> 9X4 UID FETCH 1:* UID
>
> is completely bogus. That message range is invalid, and the client
> should know that. However, the server should be responding with
> something like
>
>    9X4 BAD Invalid sequence in FETCH

I don't see why it's invalid and not just empty,
but I agree that it is inefficient for the client to
be sending it.

Russ

Re: [9fans] manual suggestions and upas/fs bug

[Lyndon Nerenberg]

    -> 9X4 UID FETCH 1:* UID
    >> 9X4 BAD Invalid sequence in FETCH

    Russ> I don't see why it's invalid and not just empty,

Because the protocol provides other means for discovering how many
messages are in a folder. The client is responsible for determining the
state of the mailbox. Having done so, it should never be in a position
where it issues a FETCH when there are no messages present. The IMAP4
protocol specification is very picky about syntax. It (and most server
implementations) don't allow for *any* reading-in of semantic (or
syntactic) behaviour. (Another example of this is the illegality of
inserting extra whitespace between protocol elements.)

Some people (usually client authors) complain that this is just
a pain in the rear. Others (usually server authors) think it's
a wonderful way to avoid ambiguous behaviour in the protocol.
(For the record: I'm a server author ;-)

If I ever get my P9 network re-built I plan to fix up the IMAP
code.

--lyndon

Re: [9fans] manual suggestions and upas/fs bug

[rob pike, esq.]

> (For the record: I'm a server author ;-)
>
> If I ever get my P9 network re-built I plan to fix up the IMAP
> code.

Great.  For the record, the upas/fs implementation of IMAP is
something of a hack; of more interest to me is the correctness
of the stand-alone IMAP server, imap4d.  (I'm using it as I write
this.)  It's been tested - and fixed - against a number of clients
but it was written by someone learning IMAP as he went, so I'm sure
it's still got some issues.

-rob

Re: [9fans] manual suggestions and upas/fs bug

[Russ Cox]

Thanks for your suggestions.  I've updated the
man pages and made the fixes to upas/fs.

I take back what I said yesterday or the day
before about IMAP being much slower than POP.
It appears that I fixed that (the overhead I was
remembering was due to constantly redialing rather
than staying dialed in, but now the code
stays dialed in).

Russ

Re: [9fans] manual suggestions and upas/fs bug

[David Swasey]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

I did not spend much time on this and am not an authority on IMAP or
upas/fs.  I could be wrong.

>Can you send me an
>IMAP protocol trace? (Along with the banner from the server.)

The trace is attached.  "8.out" is upas/fs with modified erealloc,
with the original imap4read, and with line 794 of imap.c changed to
read

	imap->debug = 1;

I modified the trace to hide my password.  Let me know if you had
something else in mind.

>What does "not like" mean in this context?

It appears the server closed the connection.  Since there is no "<-"
in the trace following the UID FETCH command, I think the first
Brdline call in imap4resp returned zero.

If it helps, I can temporarily deliver my mail elsewhere and give you
my password to this IMAP server.

-dave

[-- Attachment #2: Type: text/plain, Size: 614 bytes --]

term% ./8.out -f /imap/postoffice.srv.cs.cmu.edu
<- * OK postoffice.srv.cs.cmu.edu Cyrus IMAP4 v1.5.2 server ready
-> 9X1 LOGIN swasey ***************
<- 9X1 OK User logged in
-> 9X2 SELECT Inbox
<- * FLAGS (\Answered \Flagged \Draft \Deleted \Seen)
<- * OK [PERMANENTFLAGS (\Answered \Flagged \Draft \Deleted \Seen \*)]
<- * 0 EXISTS
<- * 0 RECENT
<- * OK [UIDVALIDITY 1031763467]
<- 9X2 OK [READ-WRITE] Completed
-> 9X3 STATUS Inbox (MESSAGES UIDVALIDITY)
<- * STATUS Inbox (MESSAGES 0 UIDVALIDITY 1031763467)
<- 9X3 OK Completed
-> 9X4 UID FETCH 1:* UID
./8.out: opening mailbox: i/o error
term%

Re: [9fans] manual suggestions and upas/fs bug

[Lyndon Nerenberg]

We have both client and server bugs here.

   -> 9X2 SELECT Inbox
   <- * FLAGS (\Answered \Flagged \Draft \Deleted \Seen)
   <- * OK [PERMANENTFLAGS (\Answered \Flagged \Draft \Deleted \Seen \*)]
   <- * 0 EXISTS
   <- * 0 RECENT
   <- * OK [UIDVALIDITY 1031763467]
   <- 9X2 OK [READ-WRITE] Completed

Notice how the server states there are no messages in the mailbox.

   -> 9X3 STATUS Inbox (MESSAGES UIDVALIDITY)

This command should not be issued, for several reasons.

    1) The SELECT response already told the client the number of messages.

    2) The SELECT response already told the client the uidvalidity
       of the mailbox.

    3) Queries to the currently selected folder should be done using
       FETCH and LIST. STATUS is for querying folders other than the
       selected one, and is not guaranteed to be fast.  (In many
       servers, selecting a folder causes the meta data about the folder
       to assembled and cached. SELECT will not use that data cache
       since its assuming you want to know something about a folder
       other than the selected one.)

   <- * STATUS Inbox (MESSAGES 0 UIDVALIDITY 1031763467)
   <- 9X3 OK Completed

And *again* the server said there were no messages, thus

   -> 9X4 UID FETCH 1:* UID

is completely bogus. That message range is invalid, and the client
should know that. However, the server should be responding with
something like

   9X4 BAD Invalid sequence in FETCH

instead of dumping core (and that's the server bug I mentioned).

--lyndon

[9fans] manual suggestions and upas/fs bug

[David Swasey]

Hi,

The malloc(2) page says:

	The call realloc(0, size) means the same as `malloc(size)'.
	Further, the call realloc(ptr, 0) means the same as
	`free(ptr)'.

I suggest you clarify the meaning of realloc(0,0).

The manual pages filter(1), mail(1), marshal(1), mlmgr(1), nedmail(1),
upasfs(4), pop3(8), send(8), and smtp(8) refer to aliasmail(1) which
does not exist.  The file /sys/man/8/INDEX.html refers to aliasmail(8)
which exists.  I suggest you decide if its aliasmail(8) or
aliasmail(1) and make the appropriate changes.

Upas/fs dies when working with an empty IMAP4 mail box.  There are two
problems, both in /sys/src/cmd/upas/fs/imap4.c:/^imap4read.  First,
when imap->nmsg is 0, the erealloc() call will fail.  Second, the
IMAP4 server I talk to (cyrus) does not like the command "UID FETCH
1:* UID" when there are no messages.  My fix was to change the
semantics of erealloc(_,0) and to avoid the offending IMAP command.
Diffs follow.

-dave

diff /n/dump/2002/0916/sys/src/cmd/upas/fs/imap4.c imap4.c
557,559c557,561
< 	imap4cmd(imap, "UID FETCH 1:* UID");
< 	if(!isokay(s = imap4resp(imap)))
< 		return s;
---
> 	if(imap->nmsg > 0){
> 		imap4cmd(imap, "UID FETCH 1:* UID");
> 		if(!isokay(s = imap4resp(imap)))
> 			return s;
> 	}
diff /n/dump/2002/0916/sys/src/cmd/upas/fs/mbox.c mbox.c
1358c1358,1362
< 	p = realloc(p, n);
---
> 	if(n==0){
> 		free(p);
> 		p = malloc(n);
> 	} else
> 		p = realloc(p, n);

Re: [9fans] manual suggestions and upas/fs bug

[Lyndon Nerenberg]

>>>>> "David" == David Swasey <swasey@cs.cmu.edu> writes:

    David> Second, the
    David> IMAP4 server I talk to (cyrus) does not like the command "UID
    David> FETCH 1:* UID" when there are no messages.

What does "not like" mean in this context? Can you send me an
IMAP protocol trace? (Along with the banner from the server.)

--lyndon

Re: [9fans] manual suggestions and upas/fs bug

[Douglas A. Gwyn]

David Swasey wrote:
> The malloc(2) page says:
>         The call realloc(0, size) means the same as `malloc(size)'.
>         Further, the call realloc(ptr, 0) means the same as
>         `free(ptr)'.
> I suggest you clarify the meaning of realloc(0,0).

If the intention is to be standard-conforming, then realloc(0,x)
is the same as malloc(x) for all x.  malloc(0) is required to
return either a null pointer or a valid heap pointer (which a
strictly conforming program is not allowed to dereference).

realloc(p,0) cannot be identical to free(p) since the former
returns a value and the latter does not.  Presumably it acts
as free(p) then does not succeed in allocating a 0-sized new
object, thus returns a null pointer.

free(0) is required to be a no-op.

> diff /n/dump/2002/0916/sys/src/cmd/upas/fs/mbox.c mbox.c
> 1358c1358,1362
> <       p = realloc(p, n);
> ---
> >       if(n==0){
> >               free(p);
> >               p = malloc(n);
> >       } else
> >               p = realloc(p, n);

p = realloc(p,n) is almost always a mistake.  If the allocation
fails, the previous pointer value in p gets replaced by a null
pointer value, so you lose the handle to the data that did not
get extended (or, rarely, shrunk).

Re: [9fans] manual suggestions and upas/fs bug

[Lucio De Re]

On Tue, Sep 17, 2002 at 08:47:45AM +0000, Douglas A. Gwyn wrote:
> > >       if(n==0){
> > >               free(p);
> > >               p = malloc(n);
> > >       } else
> > >               p = realloc(p, n);
>
> p = realloc(p,n) is almost always a mistake.  If the allocation
> fails, the previous pointer value in p gets replaced by a null
> pointer value, so you lose the handle to the data that did not
> get extended (or, rarely, shrunk).

In other words,

	if (n==0)
		panic ("zero size");
	p = realloc(p, n);

is closer to the intent here.  Better still

	char *p0 = realloc(p, n);

	if (p0 == 0)
		panic ("realloc failed");
	p = p0;

would be a lot saner.  I forget the semantics, but realloc(p, n) need
not return p even if the reallocation succeeds.  What I don't know is
what happens to the space pointed to by p, I assume it's released.

++L

Re: [9fans] manual suggestions and upas/fs bug

[Douglas A. Gwyn]

Lucio De Re wrote:
>         char *p0 = realloc(p, n);
>         if (p0 == 0)
>                 panic ("realloc failed");
>         p = p0;
> would be a lot saner.  I forget the semantics, but realloc(p, n) need
> not return p even if the reallocation succeeds.  What I don't know is
> what happens to the space pointed to by p, I assume it's released.

Actually you don't have to be very careful if you're going to abort
anyway.  (Although the returned pointer needs to be checked, even
for a shrink request.)  But if you want to recover more intelligently,
you don't want to lose the old pointer if the realloc fails; either
the old data needs to be retained (so you need to use the old pointer)
or else you want to free the old storage (otherwise you have a memory
leak, which will exacerbate the tight-storage situation).