[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Nmh-workers] mhstore dumps core
From: |
Ralph Corderoy |
Subject: |
Re: [Nmh-workers] mhstore dumps core |
Date: |
Sat, 19 Aug 2017 11:40:25 +0100 |
Hi,
David wrote:
> Ken wrote:
> > And dang, coming up with a test for the url external-body type is
> > going to be a pain.
How about
Content-Type: message/external-body; access-type="url";
url="http://google.com/robots.txt"
Could have https too. They could be skipped if a curl(1) or wget(1)
can't retrieve it from the test script successfully, e.g. no network
connection.
Or use http://httpbin.org/ instead of Google?
Or, `python2 -m SimpleHTTPServer $port' gives a HTTP server for files in
the current directory. There's a python3 variant: `python3 -m
http.server $port'. More complex needs would be a simple Python script.
Or, curl(1) supports many protocols so perhaps one of them can already
be served, e.g. POP3, or is trivial to implement, TFTP, with nc(1) or
socat(1).
> I fixed it and now valgrind doesn't report any problems.
Without David's fix, valgrind complains as expected and the fix stops
the complaint.
Invalid read of size 4
at 0x5115D70: fileno (in /usr/lib/libc-2.25.so)
by 0x11279C: openExternal (mhparse.c:2343)
by 0x11279C: openURL (mhparse.c:2797)
But I noticed there's another path out of openExternal that don't set
*fd,
2325 if (ce->ce_file) {
2326 if ((ce->ce_fp = fopen (ce->ce_file, "r")) == NULL) {
2327 content_error (ce->ce_file, ct, "unable to fopen for
reading");
2328 return NOTOK;
and tried to trigger it.
diff --git uip/mhparse.c uip/mhparse.c
index 71405616..d53cdcc1 100644
--- uip/mhparse.c
+++ uip/mhparse.c
@@ -2322,8 +2322,8 @@ openExternal (CT ct, CT cb, CE ce, char **file, int
*fd)
goto ready_already;
}
- if (ce->ce_file) {
- if ((ce->ce_fp = fopen (ce->ce_file, "r")) == NULL) {
+ if (1 || ce->ce_file) {
+ if (1 || (ce->ce_fp = fopen (ce->ce_file, "r")) == NULL) {
content_error (ce->ce_file, ct, "unable to fopen for reading");
return NOTOK;
}
@@ -2796,12 +2796,15 @@ openURL (CT ct, char **file)
switch (openExternal(e->eb_parent, e->eb_content, ce, file, &fd)) {
case NOTOK:
+ printf("fd=%d\n", fd);
return NOTOK;
case OK:
+ printf("fd=%d\n", fd);
break;
case DONE:
+ printf("fd=%d\n", fd);
return fd;
}
valgrind here, 3.13.0-2, doesn't complain about the printf, and that
prints zero. That puzzles me.
Anyway, the callers of openExternal() all do the same for the three
possible return values. NOTOK returns NOTOK, DONE returns fd, and OK
carries on.
What does the OK return value mean for openExternal()?
2333 if (find_cache (ct, rcachesw, (int *) 0, cb->c_id,
2334 cachefile, sizeof(cachefile)) != NOTOK) {
2335 if ((ce->ce_fp = fopen (cachefile, "r"))) {
2336 ce->ce_file = mh_xstrdup(cachefile);
2337 ce->ce_unlink = 0;
2338 goto ready_already;
2339 }
2340 admonish (cachefile, "unable to fopen for reading");
2341 }
2342
2343 *fd = ce->ce_fp ? fileno (ce->ce_fp) : -1;
2344 return OK;
find_cache() returns either OK or NOTOK, so `!= NOTOK' is `== OK'. If
the file is found in the cache, but we can't fopen() it, then we
admonish() to let the user know, and return OK. What's "OK" about this?
Is it just the third value to hand out of the triple?
# define NOTOK (-1)
# define OK 0
# define DONE 1
BTW, these are annoying. NOTOK is often used when the value must be -1
so it's hiding nothing, just obscuring because it's well known, e.g.
that access(2) returns -1 on error so the code would be clearer to check
against -1, not add pointless abstraction.
And then sometimes a function returning an int only returns NOTOK or OK,
so this is an inverse Boolean of success. Instead of `if
(find_cache())' where it returns non-zero to indicate success, it's `if
((find_cache() != NOTOK)', like above, or `== OK'; again, reading
overhead that also obscures it could be converted to `bool' now we have
it.
And `OK' and `DONE' are similar in English meaning, so odds are a
function that needs to distinguish should have a better definition of
what its return values mean.
--
Cheers, Ralph.
https://plus.google.com/+RalphCorderoy
- Re: [Nmh-workers] Second release candidate for nmh 1.7 is now available, (continued)
- [Nmh-workers] mhstore dumps core, norm, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core Re: Second release candidate for nmh 1.7 is now available, norm, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, Ken Hornstein, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, norm, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, Ken Hornstein, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, Ken Hornstein, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, norm, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, Ken Hornstein, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core, David Levine, 2017/08/18
- Re: [Nmh-workers] mhstore dumps core,
Ralph Corderoy <=
- Re: [Nmh-workers] mhstore dumps core, David Levine, 2017/08/19
- Re: [Nmh-workers] mhstore dumps core, Ralph Corderoy, 2017/08/19
- Re: [Nmh-workers] mhstore dumps core, David Levine, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, David Levine, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, Ralph Corderoy, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, David Levine, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, Ralph Corderoy, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, Ralph Corderoy, 2017/08/20
- Re: [Nmh-workers] mhstore dumps core, Ken Hornstein, 2017/08/23
- [Nmh-workers] Reply-to header in my replcoms, norm, 2017/08/20