Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #11

Merged
merged 9 commits into from Oct 22, 2012
Merged

Fixes #11

merged 9 commits into from Oct 22, 2012

Conversation

socketpair
Copy link
Contributor

Please review my fixes.

  1. One of the most dubious is "ROUTE_DIFF result was not used in some place in route_compare". I don't know what author mean, but this is difinitely the bug.
  2. nl_recv() refactored. Please review.

@socketpair
Copy link
Contributor Author

I thought that nf-log and ULOG are synonims. Why? because ULOG uses constant named NETLINK_NFLOG. But ULOG is deprecated in favour NF-LOG, which you already have implemented. My system still use ugly ULOG.

"-" was never used in the names of the flags. "_" was used in all places
of the library. So, I just changed the undescore to the minus.

Automatic indentation can insert spaces on either side of the minus,
so the library will be compiled, but will not be usable (in this part of the code),
as the parser will split words by white space, and the flag "admin - perm"
will never work.
Based on clang diagnostics:

1. lib/nl.c: recvmsgs(): nla filling with zeros commented.
2. lib/route/classid.c: & lib/route/pktloc.c:
   remove zero-filling of struct stat
3. lib/route/qdisc/htb.c: Fix htb_qdisc_msg_fill(): fix zero-filling
4. ematch/container.c: container_parse:
   commented why only 4 bytes are copied
   len marked as unused to eliminate compiler warning
1. all cleanup actions (like free()) now located at the end of function
2. in case of error or EOF, *buf and *creds (if given) set to NULL
   This protect from invalid code at user's side, like:
   char *buf;
   x = nl_recv(..., &buf, ...);
   if (x<=0)
      goto cleanup;
   cleanup:
      free(buf);
3. all intermediate buffers are stored into local variables, and user's
   variables only touches at the end.
1. memset around nla is unnecessary
2. calloc() is unnecessary. malloc() used instead.
@socketpair
Copy link
Contributor Author

I have done refactoring of my changes. Please check.

The last thing is to refine situation with this comment:

This changes the behavior of this function which is not acceptable. Code may exist which relies on the
NL_SOCK_PASSCRED flag to trigger the receiving of creds. We want the flag to trigger the receiving and
the result pointer to trigger returning it to the caller.

The caller may always provide a result pointer but may want to fine control receiving of creds with the flag.

The state before my changes and after described in this table:

| NL_SOCK_PASSCRED  | creds |  credentials requested? | pointer filled ?  |
|                   |       |   BEFORE   |    AFTER   |  BEFORE |  AFTER  | 
+-------------------+-------+----------------------------------------------    
|     FALSE         | NULL  |     NO     |     NO     |    NO   |   NO    |
|     FALSE         |   *   |     NO     |     NO     |    NO(!)| AS NULL |
|     TRUE          | NULL  |     YES(!) |     NO     |    NO   |   NO    |
|     TRUE          |   *   |     YES    |     YES    |   YES   |   YES   |

The exclamation mark mean wrong behaviour by my opinion. I undestand, that in theory, flags should be independent. But in practice (IMHO), some cases looks bad. So

  1. receiving credentials and not passing them to caller may gain security problems/fd leak.
  2. If caller pass creds, but forgot to set flag, caller may start (by his mistake) to use data at that pointer.
  3. If caller want to control receiving of creds, why not to require to pass NULL as creds if NL_SOCK_PASSCRED is OFF. If this breaks some existing programs, I consider this is due to bug in library. Suppose piece of code in such program:
struct ucred * creds;
nl_recv(..., &creds);
if ( sock & NL_SOCK_PASSCRED)
    handle_creds();

In my opinion, it should be changed to this:

struct ucred * creds;
nl_recv(..., sock & NL_SOCK_PASSCRED ? &creds:NULL);
if ( sock & NL_SOCK_PASSCRED)
    handle_creds();

What you think about new behaviour?

My opinion, that either such beaviour should be documented, or error code returned in middle-cases of that table.

@tgraf tgraf merged commit 420e462 into thom311:master Oct 22, 2012
@tgraf
Copy link
Contributor

tgraf commented Oct 22, 2012

I think the new creds behaviour is fine. I regard the code example you outline as a possible regression source as unlikely and consider it a bug in the application. If creds is set but the flag is not, creds shall not be requested.

@socketpair
Copy link
Contributor Author

As you think, should middle-cases of the table generate error code (instead of my behaviour)? It is minor question, but library should be clear against this.

@tgraf
Copy link
Contributor

tgraf commented Oct 22, 2012

I merged the branch that was mentioned in the email. I don't want to rebase my tree as that would force everyone to re-clone their trees. Please submit a new patches for anything that was merged incorrectly.

thom311 added a commit that referenced this pull request Oct 12, 2014
All the io_alloc() implementation unconditionally allocated
new memory, thus leaking memory if called more then once.
Fix io_alloc() implementations not to allocate new memory
if not needed.

This happens for example in link_msg_parser() which first
calls rtnl_link_set_type():

    #0  macvlan_alloc (link=0x609d50) at route/link/macvlan.c:56
    #1  0x00007ffff7b99a78 in rtnl_link_set_type (link=link@entry=0x609d50, type=type@entry=0x609a94 "macvlan") at route/link.c:2233
    #2  0x00007ffff7b99c28 in link_msg_parser (ops=<optimized out>, who=<optimized out>, n=<optimized out>, pp=0x7fffffffd870) at route/link.c:547
    #3  0x00007ffff7dea109 in nl_cache_parse (ops=0x7ffff7dd8600 <rtnl_link_ops>, who=0x603338, nlh=0x6098a0, params=0x7fffffffd870) at cache.c:914
    #4  0x00007ffff7dea15b in update_msg_parser (msg=<optimized out>, arg=<optimized out>) at cache.c:668
    #5  0x00007ffff7def7bf in nl_cb_call (msg=<optimized out>, type=<optimized out>, cb=<optimized out>) at ../include/netlink-private/netlink.h:142
    #6  recvmsgs (cb=0x6057a0, sk=0x6034c0) at nl.c:952
    #7  nl_recvmsgs_report (sk=sk@entry=0x6034c0, cb=cb@entry=0x6057a0) at nl.c:1003
    #8  0x00007ffff7defb79 in nl_recvmsgs (sk=sk@entry=0x6034c0, cb=cb@entry=0x6057a0) at nl.c:1027
    #9  0x00007ffff7de9668 in __cache_pickup (sk=0x6034c0, cache=0x603510, param=param@entry=0x7fffffffd870) at cache.c:701
    #10 0x00007ffff7dea08d in nl_cache_pickup (sk=<optimized out>, cache=<optimized out>) at cache.c:753
    #11 0x0000000000400d56 in main ()

and later ops->io_parse():

    #0  macvlan_alloc (link=0x609d50) at route/link/macvlan.c:56
    #1  0x00007ffff7baae9d in macvlan_parse (link=0x609d50, data=<optimized out>, xstats=<optimized out>) at route/link/macvlan.c:79
    #2  0x00007ffff7b99c80 in link_msg_parser (ops=<optimized out>, who=<optimized out>, n=<optimized out>, pp=0x7fffffffd870) at route/link.c:567
    #3  0x00007ffff7dea109 in nl_cache_parse (ops=0x7ffff7dd8600 <rtnl_link_ops>, who=0x603338, nlh=0x6098a0, params=0x7fffffffd870) at cache.c:914
    #4  0x00007ffff7dea15b in update_msg_parser (msg=<optimized out>, arg=<optimized out>) at cache.c:668
    #5  0x00007ffff7def7bf in nl_cb_call (msg=<optimized out>, type=<optimized out>, cb=<optimized out>) at ../include/netlink-private/netlink.h:142
    #6  recvmsgs (cb=0x6057a0, sk=0x6034c0) at nl.c:952
    #7  nl_recvmsgs_report (sk=sk@entry=0x6034c0, cb=cb@entry=0x6057a0) at nl.c:1003
    #8  0x00007ffff7defb79 in nl_recvmsgs (sk=sk@entry=0x6034c0, cb=cb@entry=0x6057a0) at nl.c:1027
    #9  0x00007ffff7de9668 in __cache_pickup (sk=0x6034c0, cache=0x603510, param=param@entry=0x7fffffffd870) at cache.c:701
    #10 0x00007ffff7dea08d in nl_cache_pickup (sk=<optimized out>, cache=<optimized out>) at cache.c:753
    #11 0x0000000000400d56 in main ()

#59

Signed-off-by: Thomas Haller <thaller@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants