Skip to content

Commit

Permalink
libsnmp: Unexport struct usmStateReference
Browse files Browse the repository at this point in the history 
Certain snmpd crashes can only be fixed by introducing a reference
count in struct usmStateReference. Unexport that structure such that
changing it does not affect the ABI.
  • Loading branch information
@bvanassche
bvanassche committed Jul 23, 2019
1 parent bfcd20d commit 39381c4
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 16 deletions.
17 changes: 1 addition & 16 deletions include/net-snmp/library/snmpusm.h
View file
Expand Up @@ -42,22 +42,7 @@ extern "C" {
/*
* Structures.
*/
struct usmStateReference {
char *usr_name;
size_t usr_name_length;
u_char *usr_engine_id;
size_t usr_engine_id_length;
oid *usr_auth_protocol;
size_t usr_auth_protocol_length;
u_char *usr_auth_key;
size_t usr_auth_key_length;
oid *usr_priv_protocol;
size_t usr_priv_protocol_length;
u_char *usr_priv_key;
size_t usr_priv_key_length;
u_int usr_sec_level;
};

struct usmStateReference;

This comment has been minimized.

Sign in to view

Copy link
@panlinux

panlinux Jun 24, 2020

Hello, Ubuntu is trying to backport this and other related changes to net-snmp 5.8 to fix the double-free bug reported in https://sourceforge.net/p/net-snmp/bugs/2923/). I see that in a follow-up commit to the above the now internal struct has a new member. You seem to be aware that just adding a member would break the ABI, as you mentioned this concern in this commit message here. Is this struct only used internally? This simple program fails to compile with the change above:

#include <stdlib.h>
#include <stdio.h>
#include <net-snmp/net-snmp-config.h>
#include <net-snmp/net-snmp-includes.h>
#include <net-snmp/library/snmpusm.h>

int main(void) {
        struct usmStateReference *mystruct;

        mystruct = usm_malloc_usmStateReference();
        printf("mystruct = %p\n", mystruct);
        printf("sizeof(*mystruct) = %ld\n", sizeof(*mystruct));
        exit(0);
}

Result:

$ gcc test.c -o test-new -I/usr/include/net-snmp -lsnmp -ggdb
test.c: In function ‘main’:
test.c:12:45: error: dereferencing pointer to incomplete type ‘struct usmStateReference’
   12 |  printf("sizeof(*mystruct) = %ld\n", sizeof(*mystruct));
      |                                             ^~~~~~~~~

Your https://github.com/net-snmp/net-snmp/blob/master/Makefile.top#L68 file implies that such a change would require a soname bump, if I'm reading it correctly:

# - If any interfaces/structures have been removed or changed since the
#   last update, increment current (+5), and set age and revision to 0. Stop.

@sergiodj ^

/*
* struct usmUser: a structure to represent a given user in a list
Expand Down
16 changes: 16 additions & 0 deletions snmplib/snmpusm.c
View file
Expand Up @@ -84,6 +84,22 @@ netsnmp_feature_child_of(usm_support, usm_all)

netsnmp_feature_require(usm_support)

struct usmStateReference {
char *usr_name;
size_t usr_name_length;
u_char *usr_engine_id;
size_t usr_engine_id_length;
oid *usr_auth_protocol;
size_t usr_auth_protocol_length;
u_char *usr_auth_key;
size_t usr_auth_key_length;
oid *usr_priv_protocol;
size_t usr_priv_protocol_length;
u_char *usr_priv_key;
size_t usr_priv_key_length;
u_int usr_sec_level;
};

oid usmNoAuthProtocol[10] = { NETSNMP_USMAUTH_BASE_OID,
NETSNMP_USMAUTH_NOAUTH };
#ifndef NETSNMP_DISABLE_MD5
Expand Down

5 comments on commit 39381c4

@bvanassche
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following grep output shows the users of the usmStateReference structure:

$ git grep -nH 'struct usmStateReference'
include/net-snmp/library/snmpusm.h:45:    struct usmStateReference;
include/net-snmp/library/snmpusm.h:93:    int             usm_clone_usmStateReference(struct usmStateReference *from,
include/net-snmp/library/snmpusm.h:94:                                                    struct usmStateReference **to);
snmplib/snmpusm.c:84:struct usmStateReference {
snmplib/snmpusm.c:274:static struct usmStateReference *
snmplib/snmpusm.c:277:    struct usmStateReference *retval;
snmplib/snmpusm.c:279:    retval = calloc(1, sizeof(struct usmStateReference));
snmplib/snmpusm.c:289:    struct usmStateReference *ref = pdu->securityStateRef;
snmplib/snmpusm.c:290:    struct usmStateReference **new_ref =
snmplib/snmpusm.c:291:        (struct usmStateReference **)&new_pdu->securityStateRef;
snmplib/snmpusm.c:311:    struct usmStateReference *ref = old;
snmplib/snmpusm.c:343:usm_set_usmStateReference_name(struct usmStateReference *ref,
snmplib/snmpusm.c:350:usm_set_usmStateReference_engine_id(struct usmStateReference *ref,
snmplib/snmpusm.c:359:usm_set_usmStateReference_auth_protocol(struct usmStateReference *ref,
snmplib/snmpusm.c:368:usm_set_usmStateReference_auth_key(struct usmStateReference *ref,
snmplib/snmpusm.c:376:usm_set_usmStateReference_priv_protocol(struct usmStateReference *ref,
snmplib/snmpusm.c:385:usm_set_usmStateReference_priv_key(struct usmStateReference *ref,
snmplib/snmpusm.c:393:usm_set_usmStateReference_sec_level(struct usmStateReference *ref,
snmplib/snmpusm.c:403:usm_clone_usmStateReference(struct usmStateReference *from, struct usmStateReference **to)
snmplib/snmpusm.c:405:    struct usmStateReference *cloned_usmStateRef;
snmplib/snmpusm.c:1481:        const struct usmStateReference *ref = secStateRef;
snmplib/snmpusm.c:1977:        const struct usmStateReference *ref = secStateRef;
snmplib/snmpusm.c:2932:    struct usmStateReference **secStateRef =
snmplib/snmpusm.c:2933:        (struct usmStateReference **) secStateRf;

So the only function in a public header file that accepts arguments of type struct usmStateReference is usm_clone_usmStateReference(). As one can see it should be possible to remove that function from the public API:

$ git grep -nHw usm_clone_usmStateReference
include/net-snmp/library/snmpusm.h:93:    int             usm_clone_usmStateReference(struct usmStateReference *from,
snmplib/snmpusm.c:299:        ret = usm_clone_usmStateReference(ref, new_ref);
snmplib/snmpusm.c:403:usm_clone_usmStateReference(struct usmStateReference *from, struct usmStateReference **to)

@sergiodj
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Bart,

It does seem that usm_clone_usmStateReference can be removed if the project wants. If the function is removed, then there will be no need to export the struct at all, IIUC.

Just reinforcing what @panlinux said, the problem here is that you had already exported the struct and its members publicly, so when you converted the struct to opaque, and then added the refcnt member to it, the ABI promise was broken. I'm definitely not an expert in the code, and it does seem that it is unlikely that a user is accessing the members of struct usmStateReference directly, but when we talk about ABIs we really can't make any assumptions and should be careful whenever there is a breakage.

I think the right thing to do here is to bump the soname of libsnmp in order to reflect the changes that were made. I apologize is that was implied in your reply :).

@bvanassche
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we really need to bump the version number. Anyway, I will bump the version number of libsnmp next week.

@sergiodj
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again,

I spent some time crafting a simple program that would validate the ABI breakage that happened. Here it is:

#include <stdlib.h>                                                                            
#include <stdio.h>                                                                             
#include <net-snmp/net-snmp-config.h>                                                          
#include <net-snmp/net-snmp-includes.h>                                                        
#include <net-snmp/library/snmpusm.h>                                                          
                                                                                               
int                                                                                            
main (int argc, char *argv[])                                                                  
{                                                                                              
  struct usmStateReference *usmref, *cloneusmref;             

  usmref = usm_malloc_usmStateReference ();

  printf ("ORIGINAL:\n\n");
  printf ("sizeof (*usmref) = %ld\n", sizeof (*usmref));
                                                                                               
  usmref->usr_name = "Test Name";
  usmref->usr_name_length = sizeof ("Test Name") - 1;
  usmref->usr_engine_id = "Test Engine";
  usmref->usr_engine_id_length = sizeof ("Test Engine") - 1;
  usmref->usr_auth_protocol_length = 0;
  usmref->usr_auth_key = "Test Auth Key";
  usmref->usr_auth_key_length = sizeof ("Test Auth Key") - 1;
  usmref->usr_priv_protocol_length = 0;
  usmref->usr_priv_key = "Test Key";
  usmref->usr_priv_key_length = sizeof ("Test Key") - 1;
  usmref->usr_sec_level = 123;

  printf ("usr_name = %s\n", usmref->usr_name); 
  printf ("usr_name_length = %lu\n", usmref->usr_name_length);
  printf ("usr_engine_id = %s\n", usmref->usr_engine_id);
  printf ("usr_engine_id_length = %lu\n", usmref->usr_engine_id_length);
  printf ("usr_auth_protocol_length = %lu\n", usmref->usr_auth_protocol_length);
  printf ("usr_auth_key = %s\n", usmref->usr_auth_key);
  printf ("usr_auth_key_length = %lu\n", usmref->usr_auth_key_length);
  printf ("usr_priv_protocol_length = %lu\n", usmref->usr_priv_protocol_length);
  printf ("usr_priv_key = %s\n", usmref->usr_priv_key);
  printf ("usr_priv_key_length = %lu\n", usmref->usr_priv_key_length);
  printf ("usr_sec_level = %u\n", usmref->usr_sec_level);

  usm_clone_usmStateReference (usmref, &cloneusmref);

  printf ("\nCLONED:\n\n");

  printf ("sizeof (*usmref) = %ld\n", sizeof (*usmref));
  printf ("usr_name = %s\n", cloneusmref->usr_name);
  printf ("usr_name_length = %lu\n", cloneusmref->usr_name_length);
  printf ("usr_engine_id = %s\n", cloneusmref->usr_engine_id);
  printf ("usr_engine_id_length = %lu\n", cloneusmref->usr_engine_id_length);
  printf ("usr_auth_protocol_length = %lu\n", cloneusmref->usr_auth_protocol_length);
  printf ("usr_auth_key = %s\n", cloneusmref->usr_auth_key);
  printf ("usr_auth_key_length = %lu\n", cloneusmref->usr_auth_key_length);
  printf ("usr_priv_protocol_length = %lu\n", cloneusmref->usr_priv_protocol_length);
  printf ("usr_priv_key = %s\n", cloneusmref->usr_priv_key);
  printf ("usr_priv_key_length = %lu\n", cloneusmref->usr_priv_key_length);
  printf ("usr_sec_level = %u\n", cloneusmref->usr_sec_level);

  exit (0);
}

When I compile it with libsnmp-5.8+dfsg-2ubuntu2, which contains the previous complete definition of struct usmStateReference, the program runs fine:

ORIGINAL:                                                                                      
                                                                                               
sizeof (*usmref) = 104                                                                         
usr_name = Test Name                                                                           
usr_name_length = 9                                                                            
usr_engine_id = Test Engine                                                                    
usr_engine_id_length = 11                                                                      
usr_auth_protocol_length = 0                                                                   
usr_auth_key = Test Auth Key                                                                   
usr_auth_key_length = 13                                                                       
usr_priv_protocol_length = 0                                                                   
usr_priv_key = Test Key                                                                        
usr_priv_key_length = 8                                                                        
usr_sec_level = 123                                                                                                                                                                            
                                                                                               
CLONED:                                                                                        
                                                                                               
sizeof (*usmref) = 104                                                                         
usr_name = Test Name                                                                           
usr_name_length = 9                                                                            
usr_engine_id = Test Engine                                                                    
usr_engine_id_length = 11                                                                      
usr_auth_protocol_length = 0                                                                   
usr_auth_key = Test Auth Key                                                                   
usr_auth_key_length = 13                                                                       
usr_priv_protocol_length = 0                                                                   
usr_priv_key = Test Key                                                                        
usr_priv_key_length = 8                                                                        
usr_sec_level = 123

However, when I update the library and run the program again (without recompiling it), I get:

ORIGINAL:

sizeof (*usmref) = 104
usr_name = Test Name
usr_name_length = 9
usr_engine_id = Test Engine
usr_engine_id_length = 11
usr_auth_protocol_length = 0
usr_auth_key = Test Auth Key
usr_auth_key_length = 13
usr_priv_protocol_length = 0
usr_priv_key = Test Key
usr_priv_key_length = 8
usr_sec_level = 123

CLONED:

sizeof (*usmref) = 104
Segmentation fault (core dumped)

Now, I understand that, according to upstream (if I understood correctly), it is unlikely that a user would be using this struct directly in the code. The problem, however, is that there was an interface exported to the user, and this interface is not being exported anymore. Bear in mind that I'm not even talking about the commit ee65591, which introduced an API breakage as well.

I hope that this is enough to justify the soname bump :-). Thanks!

@bvanassche
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library version number has been increased from 35.0.1 to 40.0.0. Please take a look.

Please sign in to comment.