Index | Thread | Search

From:
Martijn van Duren <openbsd+tech@list.imperialat.at>
Subject:
Re: snmpd/agentx compatibility issue
To:
tech@openbsd.org
Date:
Thu, 14 Mar 2024 10:39:19 +0100

Download raw body.

Thread
On Sun, 2024-02-11 at 13:36 +0100, Martijn van Duren wrote:
> Moving to tech@
> 
> On Sun, 2024-02-11 at 11:17 +0100, Steffen Christgau wrote:
> > Hi,
> > 
> > I use snmpd on OpenBSD 7.4 and try to get it work with PyAgentX3 [1] in 
> > order to provide additional data to the SNMP daemon. To test it, I use 
> > the minimal agent as provided in the PyAgentX3 readme (with the "print" 
> > function adjusted to Python 3). The respective config line for snmpd 
> > looks as simple as it could be:
> > 
> > agentx
> > 
> > Connection management and registration between the two processes work. 
> > Unfortunately, no data is returned when issuing a GET command using both 
> > OpenBSD's "snmp get" on the local machine and NET-SNMP's "snmpget" on a 
> > remote Linux machine. Instead the connection to the subagent is aborted. 
> > Same applies to a "walk".
> > 
> > The reason for that appears to be a check of the packetID in the AgentX 
> > PDU header by snmpd. From what I understand from RFC 2257, it is 
> > expected that the packetID of a response must the same as in the 
> > corresponding request. PyAgentX3 correctly does that. However, snmpd 
> > drops the response PDU during the handling of AX_PDU_TYPE_RESPONSE in 
> > ax_recv (ax.c) because it does not know the packet id, i.e. it's not in 
> > ax_packetids.
> > 
> >  From my point of view, this is due to appl_agentx_get calling ax_get 
> > with the request_id externally provided from the client (?). In ax_get, 
> > ax_pdu_header is called with that request_id. In turn, ax_pdu_header 
> > only asks for new IDs in case it is non-zero:
> > 
> >          if (packetid == 0)
> >                  packetid = ax_packetid(ax);
> >          /* possible fix
> >          else if (type == AX_PDU_TYPE_GET /*NEXT ?! */)
> >                  ax_record_packetid(packetid)
> >          */
> > 
> > So essentially, the request ID is passed through to the agentx subagent 
> > but not recorded in the array of known IDs causing the refusal of the 
> > subagent's response and finally connection close of the agentx socket.
> > 
> > Since I am not an agentx/snmp(d) expert, I'm not sure if that's a bug in 
> > snmpd or expected behavior. I would be happy if somebody could confirm 
> > either way.
> > 
> > That being said, my current fix was to disable the check for known ids 
> > in the handling of AX_PDU_TYPE_RESPONSEs, but that's apparently a bad 
> > idea. With that, however, both walking tree and getting the respective 
> > OIDs work as intended. Maybe the pseudo code above would be a better fix.
> > 
> > Regards, Steffen
> > 
> > [1] https://github.com/rprinz08/pyagentx3
> > 
> Thanks for the report and analysis. I'm also quite confident this
> explains the two elusive bug reports I've received.
> 
> First of, your analysis is correct, but I'd prefer to remove the code
> instead of creating an ax_record_packetid(). The reason this code was
> originally added to ax.c was that I wanted to lift some burden out of
> libagentx's agentx.c into ax.c to deduplicate a bit of code when it
> came to administrative packets. When I copied this over to snmpd I
> kept it, but the code was effectively never called, because ax_get,
> and ax_getnext get their packetid from application.c's requestid.
> ax_response simply copies the packetid, and other than that only
> ax_close can be called, which closes the connection after that anyway.
> So there's no reason to keep it around from that regard and ax.c is
> supposed to be a translation layer between network and C format, so
> this code also doesn't belong here from that perspective.
> 
> The reason that this triggers here and why it made the other bugs so
> elusive is because ax_response()'s call to ax_pdu_header() also called
> ax_packetid() unconditionally, which meant that an incoming packetid of
> 0 (what pyagentx3 does) would generated an (invalid) new outgoing
> packetid and would enable the `if (ax->ax_packetids != NULL)` test for
> incoming responses, making it reproducible. So how does this relate to
> the earlier reported crashes? If once in a blue moon a requestid of 0 is
> generated this code path would be enabled, resulting in EPROTO, which in
> turn puts disables the backend. Where disabling a libexec backend for
> snmpd results in snmpd shutting down.
> 
> This means that it should also be removed from libagentx, but ax's
> usage there is a bit more involved, so I need a little more time to
> build a diff for there.
> 
> OK?
> 
> martijn@
> 
I've played around a bit with the libagentx bits and there's plenty to
improve (it's almost as if one gets new insights with experience). But I
reckon this is the most straightforward diff to fix the packetid issue.

OK?

martijn@

diff db9d9b4b3c06c791a4f448a5521c340160f4705e 3f7b601986d696996ff30f574183096979f23ec7
commit - db9d9b4b3c06c791a4f448a5521c340160f4705e
commit + 3f7b601986d696996ff30f574183096979f23ec7
blob - 191892d6687fb364b54ff66dd5d572ba8346c77a
blob + ad51662c9ca9cb82afa9cf5c532c4d9c141ebc4b
--- agentx.c
+++ agentx.c
@@ -187,7 +187,7 @@ static void agentx_varbind_nosuchinstance(struct agent
 static void agentx_varbind_endofmibview(struct agentx_varbind *);
 static void agentx_varbind_error_type(struct agentx_varbind *,
     enum ax_pdu_error, int);
-static int agentx_request(struct agentx *, uint32_t,
+static struct agentx_request *agentx_request(struct agentx *,
     int (*)(struct ax_pdu *, void *), void *);
 static int agentx_request_cmp(struct agentx_request *,
     struct agentx_request *);
@@ -447,7 +447,7 @@ static int
 agentx_session_start(struct agentx_session *axs)
 {
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axr;
 
 #ifdef AX_DEBUG
 	if (ax->ax_cstate != AX_CSTATE_OPEN ||
@@ -456,18 +456,20 @@ agentx_session_start(struct agentx_session *axs)
 		agentx_log_ax_fatalx(ax, "%s: unexpected session open",
 		    __func__);
 #endif
-	packetid = ax_open(ax->ax_ax, axs->axs_timeout, &(axs->axs_oid),
-	    &(axs->axs_descr));
-	if (packetid == 0) {
+	if ((axr = agentx_request(ax, agentx_session_finalize, axs)) == NULL)
+		return -1;
+	if (ax_open(ax->ax_ax, axs->axs_timeout, axr->axr_packetid,
+	    &(axs->axs_oid), &(axs->axs_descr)) == -1) {
 		agentx_log_ax_warn(ax, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_OPEN));
 		agentx_reset(ax);
 		return -1;
 	}
-	axs->axs_packetid = packetid;
+	axs->axs_packetid = axr->axr_packetid;
 	agentx_log_ax_info(ax, "opening session");
 	axs->axs_cstate = AX_CSTATE_WAITOPEN;
-	return agentx_request(ax, packetid, agentx_session_finalize, axs);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -510,14 +512,17 @@ agentx_session_close(struct agentx_session *axs,
     enum ax_close_reason reason)
 {
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axr;
 
 #ifdef AX_DEBUG
 	if (axs->axs_cstate != AX_CSTATE_OPEN)
 		agentx_log_ax_fatalx(ax, "%s: unexpected session close",
 		    __func__);
 #endif
-	if ((packetid = ax_close(ax->ax_ax, axs->axs_id, reason)) == 0) {
+	axr = agentx_request(ax, agentx_session_close_finalize, axs);
+	if (axr == NULL)
+		return -1;
+	if (ax_close(ax->ax_ax, axs->axs_id, axr->axr_packetid, reason) == -1) {
 		agentx_log_axs_warn(axs, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_CLOSE));
 		agentx_reset(ax);
@@ -528,8 +533,8 @@ agentx_session_close(struct agentx_session *axs,
 	    ax_closereason2string(reason));
 
 	axs->axs_cstate = AX_CSTATE_WAITCLOSE;
-	return agentx_request(ax, packetid, agentx_session_close_finalize,
-	    axs);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -920,7 +925,7 @@ agentx_agentcaps_start(struct agentx_agentcaps *axa)
 	struct agentx_context *axc = axa->axa_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axr;
 
 #ifdef AX_DEBUG
 	if (axc->axc_cstate != AX_CSTATE_OPEN ||
@@ -930,9 +935,10 @@ agentx_agentcaps_start(struct agentx_agentcaps *axa)
 		    "%s: unexpected region registration", __func__);
 #endif
 
-	packetid = ax_addagentcaps(ax->ax_ax, axs->axs_id,
-	    AGENTX_CONTEXT_CTX(axc), &(axa->axa_oid), &(axa->axa_descr));
-	if (packetid == 0) {
+	if ((axr = agentx_request(ax, agentx_agentcaps_finalize, axa)) == NULL)
+		return -1;
+	if (ax_addagentcaps(ax->ax_ax, axs->axs_id, axr->axr_packetid,
+	    AGENTX_CONTEXT_CTX(axc), &(axa->axa_oid), &(axa->axa_descr)) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_ADDAGENTCAPS));
 		agentx_reset(ax);
@@ -941,8 +947,8 @@ agentx_agentcaps_start(struct agentx_agentcaps *axa)
 	agentx_log_axc_info(axc, "agentcaps %s: opening",
 	    ax_oid2string(&(axa->axa_oid)));
 	axa->axa_cstate = AX_CSTATE_WAITOPEN;
-	return agentx_request(ax, packetid, agentx_agentcaps_finalize,
-	    axa);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -983,7 +989,7 @@ agentx_agentcaps_close(struct agentx_agentcaps *axa)
 	struct agentx_context *axc = axa->axa_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axr;
 
 #ifdef AX_DEBUG
 	if (axa->axa_cstate != AX_CSTATE_OPEN)
@@ -995,9 +1001,11 @@ agentx_agentcaps_close(struct agentx_agentcaps *axa)
 	if (axs->axs_cstate == AX_CSTATE_WAITCLOSE)
 		return 0;
 
-	packetid = ax_removeagentcaps(ax->ax_ax, axs->axs_id,
-	    AGENTX_CONTEXT_CTX(axc), &(axa->axa_oid));
-	if (packetid == 0) {
+	axr = agentx_request(ax, agentx_agentcaps_close_finalize, axa);
+	if (axr == NULL)
+		return -1;
+	if (ax_removeagentcaps(ax->ax_ax, axs->axs_id, axr->axr_packetid,
+	    AGENTX_CONTEXT_CTX(axc), &(axa->axa_oid)) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_REMOVEAGENTCAPS));
 		agentx_reset(ax);
@@ -1005,8 +1013,8 @@ agentx_agentcaps_close(struct agentx_agentcaps *axa)
 	}
 	agentx_log_axc_info(axc, "agentcaps %s: closing",
 	    ax_oid2string(&(axa->axa_oid)));
-	return agentx_request(ax, packetid,
-	    agentx_agentcaps_close_finalize, axa);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -1181,7 +1189,7 @@ agentx_region_start(struct agentx_region *axr)
 	struct agentx_context *axc = axr->axr_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axreq;
 
 #ifdef AX_DEBUG
 	if (axc->axc_cstate != AX_CSTATE_OPEN ||
@@ -1191,10 +1199,11 @@ agentx_region_start(struct agentx_region *axr)
 		    "%s: unexpected region registration", __func__);
 #endif
 
-	packetid = ax_register(ax->ax_ax, 0, axs->axs_id,
+	if ((axreq = agentx_request(ax, agentx_region_finalize, axr)) == NULL)
+		return -1;
+	if (ax_register(ax->ax_ax, 0, axs->axs_id, axreq->axr_packetid,
 	    AGENTX_CONTEXT_CTX(axc), axr->axr_timeout, axr->axr_priority,
-	    0, &(axr->axr_oid), 0);
-	if (packetid == 0) {
+	    0, &(axr->axr_oid), 0) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_REGISTER));
 		agentx_reset(ax);
@@ -1203,7 +1212,8 @@ agentx_region_start(struct agentx_region *axr)
 	agentx_log_axc_info(axc, "region %s: opening",
 	    ax_oid2string(&(axr->axr_oid)));
 	axr->axr_cstate = AX_CSTATE_WAITOPEN;
-	return agentx_request(ax, packetid, agentx_region_finalize, axr);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -1267,7 +1277,7 @@ agentx_region_close(struct agentx_region *axr)
 	struct agentx_context *axc = axr->axr_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axreq;
 
 #ifdef AX_DEBUG
 	if (axr->axr_cstate != AX_CSTATE_OPEN)
@@ -1279,10 +1289,12 @@ agentx_region_close(struct agentx_region *axr)
 	if (axs->axs_cstate == AX_CSTATE_WAITCLOSE)
 		return 0;
 
-	packetid = ax_unregister(ax->ax_ax, axs->axs_id,
+	axreq = agentx_request(ax, agentx_region_close_finalize, axr);
+	if (axreq == NULL)
+		return -1;
+	if (ax_unregister(ax->ax_ax, axs->axs_id, axreq->axr_packetid,
 	    AGENTX_CONTEXT_CTX(axc), axr->axr_priority, 0, &(axr->axr_oid),
-	    0);
-	if (packetid == 0) {
+	    0) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_UNREGISTER));
 		agentx_reset(ax);
@@ -1290,8 +1302,8 @@ agentx_region_close(struct agentx_region *axr)
 	}
 	agentx_log_axc_info(axc, "region %s: closing",
 	    ax_oid2string(&(axr->axr_oid)));
-	return agentx_request(ax, packetid, agentx_region_close_finalize,
-	    axr);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -1683,7 +1695,7 @@ agentx_index_start(struct agentx_index *axi)
 	struct agentx_context *axc = axr->axr_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axreq;
 	int flags = 0;
 
 #ifdef AX_DEBUG
@@ -1705,10 +1717,11 @@ agentx_index_start(struct agentx_index *axi)
 		return 0;
 	}
 
+	if ((axreq = agentx_request(ax, agentx_index_finalize, axi)) == NULL)
+		return -1;
 	/* We might be able to bundle, but if we fail we'd have to reorganise */
-	packetid = ax_indexallocate(ax->ax_ax, flags, axs->axs_id,
-	    AGENTX_CONTEXT_CTX(axc), &(axi->axi_vb), 1);
-	if (packetid == 0) {
+	if (ax_indexallocate(ax->ax_ax, flags, axs->axs_id, axreq->axr_packetid,
+	    AGENTX_CONTEXT_CTX(axc), &(axi->axi_vb), 1) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_INDEXDEALLOCATE));
 		agentx_reset(ax);
@@ -1724,8 +1737,8 @@ agentx_index_start(struct agentx_index *axi)
 	else if (axi->axi_type == AXI_TYPE_NEW)
 		agentx_log_axc_info(axc, "index %s: allocating new index",
 		    ax_oid2string(&(axi->axi_vb.avb_oid)));
-
-	return agentx_request(ax, packetid, agentx_index_finalize, axi);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -1883,7 +1896,7 @@ agentx_index_close(struct agentx_index *axi)
 	struct agentx_context *axc = axr->axr_axc;
 	struct agentx_session *axs = axc->axc_axs;
 	struct agentx *ax = axs->axs_ax;
-	uint32_t packetid;
+	struct agentx_request *axreq;
 
 #ifdef AX_DEBUG
 	if (axi->axi_cstate != AX_CSTATE_OPEN)
@@ -1895,10 +1908,12 @@ agentx_index_close(struct agentx_index *axi)
 	if (axs->axs_cstate == AX_CSTATE_WAITCLOSE)
 		return 0;
 
+	axreq = agentx_request(ax, agentx_index_close_finalize, axi);
+	if (axreq == NULL)
+		return -1;
 	/* We might be able to bundle, but if we fail we'd have to reorganise */
-	packetid = ax_indexdeallocate(ax->ax_ax, axs->axs_id,
-	    AGENTX_CONTEXT_CTX(axc), &(axi->axi_vb), 1);
-	if (packetid == 0) {
+	if (ax_indexdeallocate(ax->ax_ax, axs->axs_id, axreq->axr_packetid,
+	    AGENTX_CONTEXT_CTX(axc), &(axi->axi_vb), 1) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_INDEXDEALLOCATE));
 		agentx_reset(ax);
@@ -1906,8 +1921,8 @@ agentx_index_close(struct agentx_index *axi)
 	}
 	agentx_log_axc_info(axc, "index %s: deallocating",
 	    ax_oid2string(&(axi->axi_vb.avb_oid)));
-	return agentx_request(ax, packetid, agentx_index_close_finalize,
-	    axi);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -2156,7 +2171,7 @@ agentx_object_start(struct agentx_object *axo)
 	char oids[1024];
 	size_t i;
 	int needregister = 0;
-	uint32_t packetid;
+	struct agentx_request *axreq;
 	uint8_t flags = AX_PDU_FLAG_INSTANCE_REGISTRATION;
 
 #ifdef AX_DEBUG
@@ -2196,10 +2211,11 @@ agentx_object_start(struct agentx_object *axo)
 		oid.aoi_id[oid.aoi_idlen++] =
 		    axo->axo_index[i]->axi_vb.avb_data.avb_int32;
 	}
-	packetid = ax_register(ax->ax_ax, flags, axs->axs_id,
+	if ((axreq = agentx_request(ax, agentx_object_finalize, axo)) == NULL)
+		return -1;
+	if (ax_register(ax->ax_ax, flags, axs->axs_id, axreq->axr_packetid,
 	    AGENTX_CONTEXT_CTX(axc), axo->axo_timeout,
-	    AX_PRIORITY_DEFAULT, 0, &oid, 0);
-	if (packetid == 0) {
+	    AX_PRIORITY_DEFAULT, 0, &oid, 0) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_REGISTER));
 		agentx_reset(ax);
@@ -2209,7 +2225,8 @@ agentx_object_start(struct agentx_object *axo)
 	agentx_log_axc_info(axc, "object %s (%s %s): opening",
 	    oids, flags ? "instance" : "region", ax_oid2string(&(oid)));
 	axo->axo_cstate = AX_CSTATE_WAITOPEN;
-	return agentx_request(ax, packetid, agentx_object_finalize, axo);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -2312,7 +2329,7 @@ agentx_object_close(struct agentx_object *axo)
 	char oids[1024];
 	size_t i;
 	int needclose = 0;
-	uint32_t packetid;
+	struct agentx_request *axr;
 	uint8_t flags = 1;
 
 #ifdef AX_DEBUG
@@ -2353,9 +2370,11 @@ agentx_object_close(struct agentx_object *axo)
 		oid.aoi_id[oid.aoi_idlen++] =
 		    axo->axo_index[i]->axi_vb.avb_data.avb_int32;
 	}
-	packetid = ax_unregister(ax->ax_ax, axs->axs_id,
-	    AGENTX_CONTEXT_CTX(axc), AX_PRIORITY_DEFAULT, 0, &oid, 0);
-	if (packetid == 0) {
+	axr = agentx_request(ax, agentx_object_close_finalize, axo);
+	if (axr == NULL)
+		return -1;
+	if (ax_unregister(ax->ax_ax, axs->axs_id, axr->axr_packetid,
+	    AGENTX_CONTEXT_CTX(axc), AX_PRIORITY_DEFAULT, 0, &oid, 0) == -1) {
 		agentx_log_axc_warn(axc, "couldn't generate %s",
 		    ax_pdutype2string(AX_PDU_TYPE_UNREGISTER));
 		agentx_reset(ax);
@@ -2364,8 +2383,8 @@ agentx_object_close(struct agentx_object *axo)
 	strlcpy(oids, ax_oid2string(&(axo->axo_oid)), sizeof(oids));
 	agentx_log_axc_info(axc, "object %s (%s %s): closing",
 	    oids, flags ? "instance" : "region", ax_oid2string(&(oid)));
-	return agentx_request(ax, packetid, agentx_object_close_finalize,
-	    axo);
+	agentx_wantwrite(ax, ax->ax_fd);
+	return 0;
 }
 
 static int
@@ -3906,40 +3925,25 @@ agentx_varbind_set_index_ipaddress(struct agentx_varbi
 #endif
 }
 
-static int
-agentx_request(struct agentx *ax, uint32_t packetid,
-    int (*cb)(struct ax_pdu *, void *), void *cookie)
+static struct agentx_request *
+agentx_request(struct agentx *ax, int (*cb)(struct ax_pdu *, void *),
+    void *cookie)
 {
 	struct agentx_request *axr;
 
-#ifdef AX_DEBUG
-	if (ax->ax_ax->ax_wblen == 0)
-		agentx_log_ax_fatalx(ax, "%s: no data to be written",
-		    __func__);
-#endif
-
 	if ((axr = calloc(1, sizeof(*axr))) == NULL) {
 		agentx_log_ax_warn(ax, "couldn't create request context");
 		agentx_reset(ax);
-		return -1;
+		return NULL;
 	}
 
-	axr->axr_packetid = packetid;
 	axr->axr_cb = cb;
 	axr->axr_cookie = cookie;
-	if (RB_INSERT(ax_requests, &(ax->ax_requests), axr) != NULL) {
-#ifdef AX_DEBUG
-		agentx_log_ax_fatalx(ax, "%s: duplicate packetid", __func__);
-#else
-		agentx_log_ax_warnx(ax, "%s: duplicate packetid", __func__);
-		free(axr);
-		agentx_reset(ax);
-		return -1;
-#endif
-	}
+	do {
+		axr->axr_packetid = arc4random();
+	} while (RB_INSERT(ax_requests, &(ax->ax_requests), axr) != NULL);
 
-	agentx_wantwrite(ax, ax->ax_fd);
-	return 0;
+	return axr;
 }
 
 static int
@@ -4023,7 +4027,7 @@ agentx_read(struct agentx *ax)
 			break;
 	}
 	if (axs == NULL) {
-		agentx_log_ax_warnx(ax, "received unexpected session: %d",
+		agentx_log_ax_warnx(ax, "received unexpected session: %u",
 		    pdu->ap_header.aph_sessionid);
 		ax_pdu_free(pdu);
 		agentx_reset(ax);
blob - 4377a82d6803d1d36c11f438f9ffbbadb944b91f
blob + 1fff36032df3331f50f005473f1cd450ff9c20a1
--- ax.c
+++ ax.c
@@ -37,8 +37,7 @@ static int ax_pdu_need(struct ax *, size_t);
 static int ax_pdu_header(struct ax *,
     enum ax_pdu_type, uint8_t, uint32_t, uint32_t, uint32_t,
     struct ax_ostring *);
-static uint32_t ax_packetid(struct ax *);
-static uint32_t ax_pdu_queue(struct ax *);
+static int ax_pdu_queue(struct ax *);
 static int ax_pdu_add_uint16(struct ax *, uint16_t);
 static int ax_pdu_add_uint32(struct ax *, uint32_t);
 static int ax_pdu_add_uint64(struct ax *, uint64_t);
@@ -89,7 +88,6 @@ ax_free(struct ax *ax)
 	close(ax->ax_fd);
 	free(ax->ax_rbuf);
 	free(ax->ax_wbuf);
-	free(ax->ax_packetids);
 	free(ax);
 }
 
@@ -103,11 +101,10 @@ ax_recv(struct ax *ax)
 	struct ax_pdu_searchrangelist *srl = NULL;
 	struct ax_pdu_varbindlist *vbl;
 	struct ax_searchrange *sr;
-	size_t rbsize, packetidx = 0, i, rawlen;
+	size_t rbsize, rawlen;
 	ssize_t nread;
 	uint8_t *u8;
 	uint8_t *rbuf;
-	int found;
 
 	/* Only read a single packet at a time to make sure libevent triggers */
 	if (ax->ax_rblen < AX_PDU_HEADER) {
@@ -267,24 +264,6 @@ ax_recv(struct ax *ax)
 		}
 		break;
 	case AX_PDU_TYPE_RESPONSE:
-		if (ax->ax_packetids != NULL) {
-			found = 0;
-			for (i = 0; ax->ax_packetids[i] != 0; i++) {
-				if (ax->ax_packetids[i] ==
-				    pdu->ap_header.aph_packetid) {
-					packetidx = i;
-					found = 1;
-				}
-			}
-			if (found) {
-				ax->ax_packetids[packetidx] =
-				    ax->ax_packetids[i - 1];
-				ax->ax_packetids[i - 1] = 0;
-			} else {
-				errno = EPROTO;
-				goto fail;
-			}
-		}
 		if (rawlen < 8) {
 			errno = EPROTO;
 			goto fail;
@@ -397,35 +376,35 @@ ax_send(struct ax *ax)
 	return ax->ax_wblen;
 }
 
-uint32_t
-ax_open(struct ax *ax, uint8_t timeout, struct ax_oid *oid,
+int
+ax_open(struct ax *ax, uint8_t timeout, uint32_t packetid, struct ax_oid *oid,
     struct ax_ostring *descr)
 {
-	if (ax_pdu_header(ax, AX_PDU_TYPE_OPEN, 0, 0, 0, 0,
+	if (ax_pdu_header(ax, AX_PDU_TYPE_OPEN, 0, 0, 0, packetid,
 	    NULL) == -1)
-		return 0;
+		return -1;
 	ax_pdu_need(ax, 4);
 	ax->ax_wbuf[ax->ax_wbtlen++] = timeout;
 	memset(&(ax->ax_wbuf[ax->ax_wbtlen]), 0, 3);
 	ax->ax_wbtlen += 3;
 	if (ax_pdu_add_oid(ax, oid, 0) == -1)
-		return 0;
+		return -1;
 	if (ax_pdu_add_str(ax, descr) == -1)
-		return 0;
+		return -1;
 
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
-ax_close(struct ax *ax, uint32_t sessionid,
+int
+ax_close(struct ax *ax, uint32_t sessionid, uint32_t packetid,
     enum ax_close_reason reason)
 {
-	if (ax_pdu_header(ax, AX_PDU_TYPE_CLOSE, 0, sessionid, 0, 0,
+	if (ax_pdu_header(ax, AX_PDU_TYPE_CLOSE, 0, sessionid, 0, packetid,
 	    NULL) == -1)
-		return 0;
+		return -1;
 
 	if (ax_pdu_need(ax, 4) == -1)
-		return 0;
+		return -1;
 	ax->ax_wbuf[ax->ax_wbtlen++] = (uint8_t)reason;
 	memset(&(ax->ax_wbuf[ax->ax_wbtlen]), 0, 3);
 	ax->ax_wbtlen += 3;
@@ -433,119 +412,120 @@ ax_close(struct ax *ax, uint32_t sessionid,
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
+int
 ax_indexallocate(struct ax *ax, uint8_t flags, uint32_t sessionid,
-    struct ax_ostring *context, struct ax_varbind *vblist, size_t nvb)
+    uint32_t packetid, struct ax_ostring *context, struct ax_varbind *vblist,
+    size_t nvb)
 {
 	if (flags & ~(AX_PDU_FLAG_NEW_INDEX | AX_PDU_FLAG_ANY_INDEX)) {
 		errno = EINVAL;
-		return 0;
+		return -1;
 	}
 
 	if (ax_pdu_header(ax, AX_PDU_TYPE_INDEXALLOCATE, flags,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 
 	if (ax_pdu_add_varbindlist(ax, vblist, nvb) == -1)
-		return 0;
+		return -1;
 
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
-ax_indexdeallocate(struct ax *ax, uint32_t sessionid,
+int
+ax_indexdeallocate(struct ax *ax, uint32_t sessionid, uint32_t packetid,
     struct ax_ostring *context, struct ax_varbind *vblist, size_t nvb)
 {
 	if (ax_pdu_header(ax, AX_PDU_TYPE_INDEXDEALLOCATE, 0,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 
 	if (ax_pdu_add_varbindlist(ax, vblist, nvb) == -1)
-		return 0;
+		return -1;
 
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
-ax_addagentcaps(struct ax *ax, uint32_t sessionid,
+int
+ax_addagentcaps(struct ax *ax, uint32_t sessionid, uint32_t packetid,
     struct ax_ostring *context, struct ax_oid *id,
     struct ax_ostring *descr)
 {
 	if (ax_pdu_header(ax, AX_PDU_TYPE_ADDAGENTCAPS, 0,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 	if (ax_pdu_add_oid(ax, id, 0) == -1)
-		return 0;
+		return -1;
 	if (ax_pdu_add_str(ax, descr) == -1)
-		return 0;
+		return -1;
 
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
-ax_removeagentcaps(struct ax *ax, uint32_t sessionid,
+int
+ax_removeagentcaps(struct ax *ax, uint32_t sessionid, uint32_t packetid,
     struct ax_ostring *context, struct ax_oid *id)
 {
 	if (ax_pdu_header(ax, AX_PDU_TYPE_REMOVEAGENTCAPS, 0,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 	if (ax_pdu_add_oid(ax, id, 0) == -1)
-		return 0;
+		return -1;
 
 	return ax_pdu_queue(ax);
 
 }
 
-uint32_t
-ax_register(struct ax *ax, uint8_t flags, uint32_t sessionid,
+int
+ax_register(struct ax *ax, uint8_t flags, uint32_t sessionid, uint32_t packetid,
     struct ax_ostring *context, uint8_t timeout, uint8_t priority,
     uint8_t range_subid, struct ax_oid *subtree, uint32_t upperbound)
 {
 	if (flags & ~(AX_PDU_FLAG_INSTANCE_REGISTRATION)) {
 		errno = EINVAL;
-		return 0;
+		return -1;
 	}
 
 	if (ax_pdu_header(ax, AX_PDU_TYPE_REGISTER, flags,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 
 	if (ax_pdu_need(ax, 4) == -1)
-		return 0;
+		return -1;
 	ax->ax_wbuf[ax->ax_wbtlen++] = timeout;
 	ax->ax_wbuf[ax->ax_wbtlen++] = priority;
 	ax->ax_wbuf[ax->ax_wbtlen++] = range_subid;
 	ax->ax_wbuf[ax->ax_wbtlen++] = 0;
 	if (ax_pdu_add_oid(ax, subtree, 0) == -1)
-		return 0;
+		return -1;
 	if (range_subid != 0) {
 		if (ax_pdu_add_uint32(ax, upperbound) == -1)
-			return 0;
+			return -1;
 	}
 
 	return ax_pdu_queue(ax);
 }
 
-uint32_t
-ax_unregister(struct ax *ax, uint32_t sessionid,
+int
+ax_unregister(struct ax *ax, uint32_t sessionid, uint32_t packetid,
     struct ax_ostring *context, uint8_t priority, uint8_t range_subid,
     struct ax_oid *subtree, uint32_t upperbound)
 {
 	if (ax_pdu_header(ax, AX_PDU_TYPE_UNREGISTER, 0,
-	    sessionid, 0, 0, context) == -1)
-		return 0;
+	    sessionid, 0, packetid, context) == -1)
+		return -1;
 
 	if (ax_pdu_need(ax, 4) == -1)
-		return 0;
+		return -1;
 	ax->ax_wbuf[ax->ax_wbtlen++] = 0;
 	ax->ax_wbuf[ax->ax_wbtlen++] = priority;
 	ax->ax_wbuf[ax->ax_wbtlen++] = range_subid;
 	ax->ax_wbuf[ax->ax_wbtlen++] = 0;
 	if (ax_pdu_add_oid(ax, subtree, 0) == -1)
-		return 0;
+		return -1;
 	if (range_subid != 0) {
 		if (ax_pdu_add_uint32(ax, upperbound) == -1)
-			return 0;
+			return -1;
 	}
 
 	return ax_pdu_queue(ax);
@@ -567,9 +547,7 @@ ax_response(struct ax *ax, uint32_t sessionid, uint32_
 
 	if (ax_pdu_add_varbindlist(ax, vblist, nvb) == -1)
 		return -1;
-	if (ax_pdu_queue(ax) == 0)
-		return -1;
-	return 0;
+	return ax_pdu_queue(ax);
 }
 
 void
@@ -954,23 +932,22 @@ ax_oid_add(struct ax_oid *oid, uint32_t value)
 	return 0;
 }
 
-static uint32_t
+static int
 ax_pdu_queue(struct ax *ax)
 {
 	struct ax_pdu_header header;
-	uint32_t packetid, plength;
+	uint32_t plength;
 	size_t wbtlen = ax->ax_wbtlen;
 
 	header.aph_flags = ax->ax_byteorder == AX_BYTE_ORDER_BE ?
 	    AX_PDU_FLAG_NETWORK_BYTE_ORDER : 0;
-	packetid = ax_pdutoh32(&header, &(ax->ax_wbuf[ax->ax_wblen + 12]));
 	plength = (ax->ax_wbtlen - ax->ax_wblen) - AX_PDU_HEADER;
 	ax->ax_wbtlen = ax->ax_wblen + 16;
 	(void)ax_pdu_add_uint32(ax, plength);
 
 	ax->ax_wblen = ax->ax_wbtlen = wbtlen;
 
-	return packetid;
+	return 0;
 }
 
 static int
@@ -993,8 +970,6 @@ ax_pdu_header(struct ax *ax, enum ax_pdu_type type, ui
 		flags |= AX_PDU_FLAG_NETWORK_BYTE_ORDER;
 	ax->ax_wbuf[ax->ax_wbtlen++] = flags;
 	ax->ax_wbuf[ax->ax_wbtlen++] = 0;
-	if (packetid == 0)
-		packetid = ax_packetid(ax);
 	if (ax_pdu_add_uint32(ax, sessionid) == -1 ||
 	    ax_pdu_add_uint32(ax, transactionid) == -1 ||
 	    ax_pdu_add_uint32(ax, packetid) == -1 ||
@@ -1009,40 +984,6 @@ ax_pdu_header(struct ax *ax, enum ax_pdu_type type, ui
 	return 0;
 }
 
-static uint32_t
-ax_packetid(struct ax *ax)
-{
-	uint32_t packetid, *packetids;
-	size_t npackets = 0, i;
-	int found;
-
-	if (ax->ax_packetids != NULL) {
-		for (npackets = 0; ax->ax_packetids[npackets] != 0; npackets++)
-			continue;
-	}
-	if (ax->ax_packetidsize == 0 || npackets == ax->ax_packetidsize - 1) {
-		packetids = recallocarray(ax->ax_packetids, ax->ax_packetidsize,
-		    ax->ax_packetidsize + 25, sizeof(*packetids));
-		if (packetids == NULL)
-			return 0;
-		ax->ax_packetidsize += 25;
-		ax->ax_packetids = packetids;
-	}
-	do {
-		found = 0;
-		packetid = arc4random();
-		for (i = 0; ax->ax_packetids[i] != 0; i++) {
-			if (ax->ax_packetids[i] == packetid) {
-				found = 1;
-				break;
-			}
-		}
-	} while (packetid == 0 || found);
-	ax->ax_packetids[npackets] = packetid;
-
-	return packetid;
-}
-
 static int
 ax_pdu_add_uint16(struct ax *ax, uint16_t value)
 {
blob - f4de378f238abece8162483a3f3c9e731666e23e
blob + 0f9aeda495ab58dacca43e89b5c098fef806fb30
--- ax.h
+++ ax.h
@@ -122,8 +122,6 @@ struct ax {
 	size_t ax_wblen;
 	size_t ax_wbtlen;
 	size_t ax_wbsize;
-	uint32_t *ax_packetids;
-	size_t ax_packetidsize;
 };
 
 #ifndef AX_PRIMITIVE
@@ -203,21 +201,21 @@ struct ax *ax_new(int);
 void ax_free(struct ax *);
 struct ax_pdu *ax_recv(struct ax *);
 ssize_t ax_send(struct ax *);
-uint32_t ax_open(struct ax *, uint8_t, struct ax_oid *,
+int ax_open(struct ax *, uint8_t, uint32_t, struct ax_oid *,
     struct ax_ostring *);
-uint32_t ax_close(struct ax *, uint32_t, enum ax_close_reason);
-uint32_t ax_indexallocate(struct ax *, uint8_t, uint32_t,
+int ax_close(struct ax *, uint32_t, uint32_t, enum ax_close_reason);
+int ax_indexallocate(struct ax *, uint8_t, uint32_t, uint32_t,
     struct ax_ostring *, struct ax_varbind *, size_t);
-uint32_t ax_indexdeallocate(struct ax *, uint32_t,
+int ax_indexdeallocate(struct ax *, uint32_t, uint32_t,
     struct ax_ostring *, struct ax_varbind *, size_t);
-uint32_t ax_addagentcaps(struct ax *, uint32_t, struct ax_ostring *,
+int ax_addagentcaps(struct ax *, uint32_t, uint32_t, struct ax_ostring *,
     struct ax_oid *, struct ax_ostring *);
-uint32_t ax_removeagentcaps(struct ax *, uint32_t,
+int ax_removeagentcaps(struct ax *, uint32_t, uint32_t,
     struct ax_ostring *, struct ax_oid *);
-uint32_t ax_register(struct ax *, uint8_t, uint32_t,
+int ax_register(struct ax *, uint8_t, uint32_t, uint32_t,
     struct ax_ostring *, uint8_t, uint8_t, uint8_t, struct ax_oid *,
     uint32_t);
-uint32_t ax_unregister(struct ax *, uint32_t, struct ax_ostring *,
+int ax_unregister(struct ax *, uint32_t, uint32_t, struct ax_ostring *,
     uint8_t, uint8_t, struct ax_oid *, uint32_t);
 int ax_response(struct ax *, uint32_t, uint32_t, uint32_t,
     uint32_t, uint16_t, uint16_t, struct ax_varbind *, size_t);