#71 Failure to connect to server causes core dump

closed-fixed
nobody
None
5
2004-04-07
2004-03-04
pkeusem
No

This is in liferea-0.4.6d.

A failure to connect to a server in the DownloadFeed
function
can cause a core dump if the connectresult variable is -1.

If connectresult is -1 in the call to snprintf at line
670 in netio.c, strerrro returns NULL, causing snprintf
to drop core.

Discussion

  • Oliver Feiler
    Oliver Feiler
    2004-03-04

    Logged In: YES
    user_id=26908

    Which operating system are you running on?

    strerror(-1) should never return a NULL pointer. ISO C99
    section 7.21.6.2 says for strerror:

    "The strerror function maps the number in errnum to a
    message string. Typically, the values for errnum come from
    errno, but strerror shall map any value of type int to a
    message."

    So "any value of type int" surely includes -1. Tested and
    works like that on Linux, OpenBSD, FreeBSD and OS X.

     
  • pkeusem
    pkeusem
    2004-03-04

    Logged In: YES
    user_id=990411

    I running on Solaris 8. The man page for strerror says:

    ...
    RETURN VALUES
    The strerror() function returns NULL if errnum is
    out-of-
    range.

    It's probably not the correct thing to do but there it is.

     
  • Oliver Feiler
    Oliver Feiler
    2004-03-04

    Logged In: YES
    user_id=26908

    Actually the code
    printf ("%s", NULL);
    works fine and is perfectly legal IIRC. It outputs the
    string "(null)".

    The following change should make Solaris behave the same.

    - snprintf (tmp ... strerror(connectresult));
    + snprintf (tmp ... (strerror(connectresult) ?
    strerror(connectresult) : "(null)"));

    But why do you get errno -1 in the first place? Something
    else is going on and trashing errno. connect() should not
    return something strerror chokes on. Can you try on Solaris
    9 maybe? For which server you try to connect do you get this
    error?

     
  • pkeusem
    pkeusem
    2004-03-05

    Logged In: YES
    user_id=990411

    I don't believe

    printf("%s", NULL);

    is very portable but aside from that, as far as connect failing.
    It was a transient error. I believe groklaw was the site it
    failed
    on. It worked the next time I tried it.

    I didn't realize this before but now that I look at it, the
    bug is that
    connectresult is being used instead of errno. connect() returns
    -1 when there is an error. The value of errno should get stored
    away after the connect attempt and that stored value should be
    passed to strerror instead of connectresult.
    was -1, sorry.

     
  • Oliver Feiler
    Oliver Feiler
    2004-03-05

    Logged In: YES
    user_id=26908

    I never saw passing NULL to *printf fail. It's just an
    excuse for lazy Sun developers. :P

    The variable connectresult contains the actual value of
    errno. See the code:
    getsockopt(*my_socket, SOL_SOCKET, SO_ERROR, &connectresult,
    &len);

    some lines later in NetConnect(). All this is needed,
    because the socket is in NONBLOCK mode which is what I need
    in Snownews.

    Hm, I see that there is a "return 3;" after "if (errno !=
    EINPROGRESS)", but that is just a workaround as well (BSDs
    seem to do this when connecting to localhost). Just checking
    for NULL before snprintf is fine, I don't care too much to
    include even more workarounds. So Solaris never sets errno
    to EINPROGRESS which it should be after connect()ing a
    non-blocking socket?

     
  • Oliver Feiler
    Oliver Feiler
    2004-03-05

    Logged In: YES
    user_id=26908

    Ok, I see, the Linux manpage of printf function family says
    that for snprintf different standards tell different things.
    C99 allows arg passed to snprintf to be NULL while SUSv2
    says the opposite.

    The manpages for Sol I found on docs.sun.com also says that
    connect on nonblock sockets sets EINPROGRESS. So it should
    be that getsockopt writes -1 into connectresult. I know from
    someone else who runs the code on Solaris 9 that, in theory,
    it does what it's supposed to do so this error is just
    weird. I'd say just catch the NULL ptr and forget about it. ;)

     
  • Lars Windolf
    Lars Windolf
    2004-04-07

    Logged In: YES
    user_id=834800

    Fixes should be included in 0.4.7.

    Please reopen if you don't agree.

     
  • Lars Windolf
    Lars Windolf
    2004-04-07

    • status: open --> closed-fixed