nmh-workers
[Top] [All Lists]

Re: [Nmh-workers] mhstore dumps core

2017-08-19 05:40:47
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

_______________________________________________
Nmh-workers mailing list
Nmh-workers(_at_)nongnu(_dot_)org
https://lists.nongnu.org/mailman/listinfo/nmh-workers

<Prev in Thread] Current Thread [Next in Thread>