From acc21c4c0f95ed60421bc4a5a74caddf1ce62e55 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 4 Feb 2019 07:39:03 +0000 Subject: [PATCH] Stop using unqualified {GET,PUT}_32BIT. Those were a reasonable abbreviation when the code almost never had to deal with little-endian numbers, but they've crept into enough places now (e.g. the ECC formatting) that I think I'd now prefer that every use of the integer read/write macros was clearly marked with its endianness. So all uses of GET_??BIT and PUT_??BIT are now qualified. The special versions in x11fwd.c, which used variable endianness because so does the X11 protocol, are suffixed _X11 to make that clear, and where that pushed line lengths over 80 characters I've taken the opportunity to name a local variable to remind me of what that extra parameter actually does. --- agentf.c | 2 +- import.c | 8 ++++---- misc.h | 4 ---- pageant.c | 12 ++++++------ sesschan.c | 2 +- sftp.c | 2 +- sftpcommon.c | 2 +- ssh1bpp.c | 6 +++--- ssh2bpp-bare.c | 2 +- ssh2bpp.c | 10 +++++----- sshpubk.c | 4 ++-- sshshare.c | 20 ++++++++++---------- unix/uxagentc.c | 2 +- unix/uxsftpserver.c | 8 ++++---- windows/winpgnt.c | 4 ++-- windows/winpgntc.c | 2 +- x11fwd.c | 24 +++++++++++++----------- 17 files changed, 56 insertions(+), 58 deletions(-) diff --git a/agentf.c b/agentf.c index 4b859f44..5bac3797 100644 --- a/agentf.c +++ b/agentf.c @@ -72,7 +72,7 @@ static void agentf_try_forward(agentf *af) break; /* not even a length field available yet */ bufchain_fetch(&af->inbuffer, msglen, 4); - length = GET_32BIT(msglen); + length = GET_32BIT_MSB_FIRST(msglen); if (length > AGENT_MAX_MSGLEN-4) { /* diff --git a/import.c b/import.c index 2b4cea5a..499d5455 100644 --- a/import.c +++ b/import.c @@ -2273,8 +2273,8 @@ static bool sshcom_write( for (i = 0; i < nnumbers; i++) put_mp_sshcom_from_string(outblob, numbers[i].ptr, numbers[i].len); /* Now wrap up the encrypted payload. */ - PUT_32BIT(outblob->s + lenpos + 4, - outblob->len - (lenpos + 8)); + PUT_32BIT_MSB_FIRST(outblob->s + lenpos + 4, + outblob->len - (lenpos + 8)); /* Pad encrypted blob to a multiple of cipher block size. */ if (passphrase) { int padding = -(outblob->len - (lenpos+4)) & 7; @@ -2286,9 +2286,9 @@ static bool sshcom_write( cipherlen = outblob->len - (lenpos + 4); assert(!passphrase || cipherlen % 8 == 0); /* Wrap up the encrypted blob string. */ - PUT_32BIT(outblob->s + lenpos, cipherlen); + PUT_32BIT_MSB_FIRST(outblob->s + lenpos, cipherlen); /* And finally fill in the total length field. */ - PUT_32BIT(outblob->s + 4, outblob->len); + PUT_32BIT_MSB_FIRST(outblob->s + 4, outblob->len); /* * Encrypt the key. diff --git a/misc.h b/misc.h index 9e34addd..931fd32f 100644 --- a/misc.h +++ b/misc.h @@ -285,16 +285,12 @@ void debug_memdump(const void *buf, int len, bool L); ((uint32_t)(unsigned char)(cp)[2] << 8) | \ ((uint32_t)(unsigned char)(cp)[3])) -#define GET_32BIT(cp) GET_32BIT_MSB_FIRST(cp) - #define PUT_32BIT_MSB_FIRST(cp, value) ( \ (cp)[0] = (unsigned char)((value) >> 24), \ (cp)[1] = (unsigned char)((value) >> 16), \ (cp)[2] = (unsigned char)((value) >> 8), \ (cp)[3] = (unsigned char)(value) ) -#define PUT_32BIT(cp, value) PUT_32BIT_MSB_FIRST(cp, value) - #define GET_64BIT_MSB_FIRST(cp) \ (((uint64_t)(unsigned char)(cp)[0] << 56) | \ ((uint64_t)(unsigned char)(cp)[1] << 48) | \ diff --git a/pageant.c b/pageant.c index be2dd01d..24f4f9e3 100644 --- a/pageant.c +++ b/pageant.c @@ -770,7 +770,7 @@ static void pageant_conn_receive(Plug *plug, int urgent, char *data, int len) pc->lenbuf[pc->got++] = c; } - pc->len = GET_32BIT(pc->lenbuf); + pc->len = GET_32BIT_MSB_FIRST(pc->lenbuf); pc->got = 0; pc->real_packet = (pc->len < AGENT_MAX_MSGLEN-4); @@ -796,7 +796,7 @@ static void pageant_conn_receive(Plug *plug, int urgent, char *data, int len) pc->logfn ? pageant_conn_log : NULL); } - PUT_32BIT(reply->s, reply->len - 4); + PUT_32BIT_MSB_FIRST(reply->s, reply->len - 4); sk_write(pc->connsock, reply->s, reply->len); strbuf_free(reply); @@ -1056,7 +1056,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, strbuf_free(blob); return PAGEANT_ACTION_FAILURE; } - PUT_32BIT(blob->s, blob->len - 4); + PUT_32BIT_MSB_FIRST(blob->s, blob->len - 4); keylist = pageant_get_keylist2(&keylistlen); } if (keylist) { @@ -1066,7 +1066,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, strbuf_free(blob); return PAGEANT_ACTION_FAILURE; } - nkeys = toint(GET_32BIT(keylist)); + nkeys = toint(GET_32BIT_MSB_FIRST(keylist)); if (nkeys < 0) { *retstr = dupstr("Received broken key list from agent"); sfree(keylist); @@ -1103,7 +1103,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, strbuf_free(blob); return PAGEANT_ACTION_FAILURE; } - n = GET_32BIT(p); + n = GET_32BIT_MSB_FIRST(p); p += 4; keylistlen -= 4; @@ -1125,7 +1125,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, strbuf_free(blob); return PAGEANT_ACTION_FAILURE; } - n = GET_32BIT(p); + n = GET_32BIT_MSB_FIRST(p); p += 4; keylistlen -= 4; diff --git a/sesschan.c b/sesschan.c index 1ca085d1..14eee41e 100644 --- a/sesschan.c +++ b/sesschan.c @@ -660,7 +660,7 @@ static int sftp_chan_send(Channel *chan, bool is_stderr, struct sftp_packet *pkt, *reply; bufchain_fetch(&sess->subsys_input, lenbuf, 4); - pktlen = GET_32BIT(lenbuf); + pktlen = GET_32BIT_MSB_FIRST(lenbuf); if (bufchain_size(&sess->subsys_input) - 4 < pktlen) break; /* wait for more data */ diff --git a/sftp.c b/sftp.c index 3bd4d15e..7a6d5441 100644 --- a/sftp.c +++ b/sftp.c @@ -38,7 +38,7 @@ struct sftp_packet *sftp_recv(void) if (!sftp_recvdata(x, 4)) return NULL; - pkt = sftp_recv_prepare(GET_32BIT(x)); + pkt = sftp_recv_prepare(GET_32BIT_MSB_FIRST(x)); if (!sftp_recvdata(pkt->data, pkt->length)) { sftp_pkt_free(pkt); diff --git a/sftpcommon.c b/sftpcommon.c index 57e737fc..aef6c247 100644 --- a/sftpcommon.c +++ b/sftpcommon.c @@ -111,7 +111,7 @@ void sftp_pkt_free(struct sftp_packet *pkt) void sftp_send_prepare(struct sftp_packet *pkt) { - PUT_32BIT(pkt->data, pkt->length - 4); + PUT_32BIT_MSB_FIRST(pkt->data, pkt->length - 4); if (pkt->length >= 5) { /* Rewrite the type code, in case the caller changed its mind * about pkt->type since calling sftp_pkt_init */ diff --git a/ssh1bpp.c b/ssh1bpp.c index 52610489..733c77f9 100644 --- a/ssh1bpp.c +++ b/ssh1bpp.c @@ -181,7 +181,7 @@ static void ssh1_bpp_handle_input(BinaryPacketProtocol *bpp) ssh_cipher_decrypt(s->cipher_in, s->data, s->biglen); s->realcrc = crc32_ssh1(make_ptrlen(s->data, s->biglen - 4)); - s->gotcrc = GET_32BIT(s->data + s->biglen - 4); + s->gotcrc = GET_32BIT_MSB_FIRST(s->data + s->biglen - 4); if (s->gotcrc != s->realcrc) { ssh_sw_abort(s->bpp.ssh, "Incorrect CRC received on packet"); crStopV; @@ -332,8 +332,8 @@ static void ssh1_bpp_format_packet(struct ssh1_bpp_state *s, PktOut *pkt) random_read(pkt->data + pktoffs, 4+8 - pktoffs); crc = crc32_ssh1( make_ptrlen(pkt->data + pktoffs + 4, biglen - 4)); /* all ex len */ - PUT_32BIT(pkt->data + pktoffs + 4 + biglen - 4, crc); - PUT_32BIT(pkt->data + pktoffs, len); + PUT_32BIT_MSB_FIRST(pkt->data + pktoffs + 4 + biglen - 4, crc); + PUT_32BIT_MSB_FIRST(pkt->data + pktoffs, len); if (s->cipher_out) ssh_cipher_encrypt(s->cipher_out, pkt->data + pktoffs + 4, biglen); diff --git a/ssh2bpp-bare.c b/ssh2bpp-bare.c index 8db2f7f7..292803f8 100644 --- a/ssh2bpp-bare.c +++ b/ssh2bpp-bare.c @@ -170,7 +170,7 @@ static void ssh2_bare_bpp_format_packet(struct ssh2_bare_bpp_state *s, s->outgoing_sequence++; /* only for diagnostics, really */ - PUT_32BIT(pkt->data, pkt->length - 4); + PUT_32BIT_MSB_FIRST(pkt->data, pkt->length - 4); bufchain_add(s->bpp.out_raw, pkt->data, pkt->length); } diff --git a/ssh2bpp.c b/ssh2bpp.c index 241a4818..68368f24 100644 --- a/ssh2bpp.c +++ b/ssh2bpp.c @@ -343,7 +343,7 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) /* See if that gives us a valid packet. */ if (ssh2_mac_verresult(s->in.mac, s->buf + s->packetlen) && - ((s->len = toint(GET_32BIT(s->buf))) == + ((s->len = toint(GET_32BIT_MSB_FIRST(s->buf))) == s->packetlen-4)) break; if (s->packetlen >= (long)OUR_V2_PACKETLIMIT) { @@ -383,9 +383,9 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) memcpy(len, s->buf, 4); ssh_cipher_decrypt_length( s->in.cipher, len, 4, s->in.sequence); - s->len = toint(GET_32BIT(len)); + s->len = toint(GET_32BIT_MSB_FIRST(len)); } else { - s->len = toint(GET_32BIT(s->buf)); + s->len = toint(GET_32BIT_MSB_FIRST(s->buf)); } /* @@ -450,7 +450,7 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) /* * Now get the length figure. */ - s->len = toint(GET_32BIT(s->buf)); + s->len = toint(GET_32BIT_MSB_FIRST(s->buf)); /* * _Completely_ silly lengths should be stomped on before they @@ -729,7 +729,7 @@ static void ssh2_bpp_format_packet_inner(struct ssh2_bpp_state *s, PktOut *pkt) put_byte(pkt, 0); /* make space for random padding */ random_read(pkt->data + origlen, padding); pkt->data[4] = padding; - PUT_32BIT(pkt->data, origlen + padding - 4); + PUT_32BIT_MSB_FIRST(pkt->data, origlen + padding - 4); /* Encrypt length if the scheme requires it */ if (s->out.cipher && diff --git a/sshpubk.c b/sshpubk.c index 40382656..18a62fb9 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -950,7 +950,7 @@ bool rfc4716_loadpub(FILE *fp, char **algorithm, error = "not enough data in SSH-2 public key file"; goto error; } - alglen = toint(GET_32BIT(pubblob)); + alglen = toint(GET_32BIT_MSB_FIRST(pubblob)); if (alglen < 0 || alglen > pubbloblen-4) { error = "invalid algorithm prefix in SSH-2 public key file"; goto error; @@ -1021,7 +1021,7 @@ bool openssh_loadpub(FILE *fp, char **algorithm, */ alglen = strlen(line); if (pubbloblen < alglen + 4 || - GET_32BIT(pubblob) != alglen || + GET_32BIT_MSB_FIRST(pubblob) != alglen || 0 != memcmp(pubblob + 4, line, alglen)) { error = "key algorithms do not match in OpenSSH public key file"; goto error; diff --git a/sshshare.c b/sshshare.c index 664a622e..f10a29d6 100644 --- a/sshshare.c +++ b/sshshare.c @@ -776,7 +776,7 @@ static void send_packet_to_downstream(struct ssh_sharing_connstate *cs, put_data(packet, data.ptr, this_len); data.ptr = (const char *)data.ptr + this_len; data.len -= this_len; - PUT_32BIT(packet->s, packet->len-4); + PUT_32BIT_MSB_FIRST(packet->s, packet->len-4); sk_write(cs->sock, packet->s, packet->len); strbuf_free(packet); } while (data.len > 0); @@ -788,7 +788,7 @@ static void send_packet_to_downstream(struct ssh_sharing_connstate *cs, put_uint32(packet, 0); /* placeholder for length field */ put_byte(packet, type); put_data(packet, pkt, pktlen); - PUT_32BIT(packet->s, packet->len-4); + PUT_32BIT_MSB_FIRST(packet->s, packet->len-4); sk_write(cs->sock, packet->s, packet->len); strbuf_free(packet); } @@ -1052,7 +1052,7 @@ void share_xchannel_confirmation(struct ssh_sharing_connstate *cs, xc->msghead = msg->next; if (msg->datalen >= 4) - PUT_32BIT(msg->data, chan->downstream_id); + PUT_32BIT_MSB_FIRST(msg->data, chan->downstream_id); send_packet_to_downstream(cs, msg->type, msg->data, msg->datalen, chan); @@ -1239,7 +1239,7 @@ void share_got_pkt_from_server(ssh_sharing_connstate *cs, int type, */ unsigned char *rewritten = snewn(pktlen, unsigned char); memcpy(rewritten, pkt, pktlen); - PUT_32BIT(rewritten + id_pos, chan->downstream_id); + PUT_32BIT_MSB_FIRST(rewritten + id_pos, chan->downstream_id); send_packet_to_downstream(cs, type, rewritten, pktlen, chan); sfree(rewritten); @@ -1248,8 +1248,8 @@ void share_got_pkt_from_server(ssh_sharing_connstate *cs, int type, */ if (type == SSH2_MSG_CHANNEL_OPEN_CONFIRMATION) { if (chan->state == UNACKNOWLEDGED && pktlen >= 8) { - share_channel_set_server_id(cs, chan, GET_32BIT(pkt+4), - OPEN); + share_channel_set_server_id( + cs, chan, GET_32BIT_MSB_FIRST(pkt+4), OPEN); if (!cs->sock) { /* Retry cleaning up this connection, so that we * can send an immediate CLOSE on this channel for @@ -1490,7 +1490,7 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, goto confused; } share_add_channel(cs, old_id, new_id, 0, UNACKNOWLEDGED, maxpkt); - PUT_32BIT(pkt + id_pos, new_id); + PUT_32BIT_MSB_FIRST(pkt + id_pos, new_id); ssh_send_packet_from_downstream(cs->parent->cl, cs->id, type, pkt, pktlen, NULL); break; @@ -1523,7 +1523,7 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, goto confused; } - PUT_32BIT(pkt + id_pos, new_id); + PUT_32BIT_MSB_FIRST(pkt + id_pos, new_id); chan = share_add_channel(cs, old_id, new_id, server_id, OPEN, maxpkt); @@ -1532,7 +1532,7 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, type, pkt, pktlen, NULL); share_remove_halfchannel(cs, hc); } else if (xc) { - unsigned downstream_window = GET_32BIT(pkt + 8); + unsigned downstream_window = GET_32BIT_MSB_FIRST(pkt + 8); if (downstream_window < 256) { err = dupprintf("Initial window size for x11 channel must be at least 256 (got %u)", downstream_window); goto confused; @@ -1808,7 +1808,7 @@ static void share_receive(Plug *plug, int urgent, char *data, int len) crGetChar(c); cs->recvbuf[cs->recvlen++] = c; } - cs->curr_packetlen = toint(GET_32BIT(cs->recvbuf) + 4); + cs->curr_packetlen = toint(GET_32BIT_MSB_FIRST(cs->recvbuf) + 4); if (cs->curr_packetlen < 5 || cs->curr_packetlen > sizeof(cs->recvbuf)) { char *buf = dupprintf("Bad packet length %u\n", diff --git a/unix/uxagentc.c b/unix/uxagentc.c index edd05d79..42c1b373 100644 --- a/unix/uxagentc.c +++ b/unix/uxagentc.c @@ -73,7 +73,7 @@ static bool agent_try_read(agent_pending_query *conn) } conn->retlen += ret; if (conn->retsize == 4 && conn->retlen == 4) { - conn->retsize = toint(GET_32BIT(conn->retbuf) + 4); + conn->retsize = toint(GET_32BIT_MSB_FIRST(conn->retbuf) + 4); if (conn->retsize <= 0) { conn->retbuf = NULL; conn->retlen = 0; diff --git a/unix/uxsftpserver.c b/unix/uxsftpserver.c index 17ab8c0b..7c485a4a 100644 --- a/unix/uxsftpserver.c +++ b/unix/uxsftpserver.c @@ -93,8 +93,8 @@ static void uss_return_handle_raw( UnixSftpServer *uss, SftpReplyBuilder *reply, int index, unsigned seq) { unsigned char handlebuf[8]; - PUT_32BIT(handlebuf, index); - PUT_32BIT(handlebuf + 4, seq); + PUT_32BIT_MSB_FIRST(handlebuf, index); + PUT_32BIT_MSB_FIRST(handlebuf + 4, seq); des_encrypt_xdmauth(uss->handlekey, handlebuf, 8); fxp_reply_handle(reply, make_ptrlen(handlebuf, 8)); } @@ -108,8 +108,8 @@ static bool uss_decode_handle( return false; memcpy(handlebuf, handle.ptr, 8); des_decrypt_xdmauth(uss->handlekey, handlebuf, 8); - *index = toint(GET_32BIT(handlebuf)); - *seq = GET_32BIT(handlebuf + 4); + *index = toint(GET_32BIT_MSB_FIRST(handlebuf)); + *seq = GET_32BIT_MSB_FIRST(handlebuf + 4); return true; } diff --git a/windows/winpgnt.c b/windows/winpgnt.c index 68462e10..b777fca5 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -891,7 +891,7 @@ static char *answer_filemapping_message(const char *mapname) goto cleanup; } - msglen = GET_32BIT((unsigned char *)mapaddr); + msglen = GET_32BIT_MSB_FIRST((unsigned char *)mapaddr); #ifdef DEBUG_IPC debug("msg length=%08x, msg type=%02x\n", @@ -925,7 +925,7 @@ static char *answer_filemapping_message(const char *mapname) } /* Write in the initial length field, and we're done. */ - PUT_32BIT(((unsigned char *)mapaddr), reply.len); + PUT_32BIT_MSB_FIRST(((unsigned char *)mapaddr), reply.len); cleanup: /* expectedsid has the lifetime of the program, so we don't free it */ diff --git a/windows/winpgntc.c b/windows/winpgntc.c index 08f5066a..db1cda6f 100644 --- a/windows/winpgntc.c +++ b/windows/winpgntc.c @@ -109,7 +109,7 @@ agent_pending_query *agent_query( */ id = SendMessage(hwnd, WM_COPYDATA, (WPARAM) NULL, (LPARAM) &cds); if (id > 0) { - retlen = 4 + GET_32BIT(p); + retlen = 4 + GET_32BIT_MSB_FIRST(p); ret = snewn(retlen, unsigned char); if (ret) { memcpy(ret, p, retlen); diff --git a/x11fwd.c b/x11fwd.c index b52cf004..15ae16c4 100644 --- a/x11fwd.c +++ b/x11fwd.c @@ -12,10 +12,10 @@ #include "sshchan.h" #include "tree234.h" -#define GET_16BIT(endian, cp) \ +#define GET_16BIT_X11(endian, cp) \ (endian=='B' ? GET_16BIT_MSB_FIRST(cp) : GET_16BIT_LSB_FIRST(cp)) -#define PUT_16BIT(endian, cp, val) \ +#define PUT_16BIT_X11(endian, cp, val) \ (endian=='B' ? PUT_16BIT_MSB_FIRST(cp, val) : PUT_16BIT_LSB_FIRST(cp, val)) const char *const x11_authnames[] = { @@ -881,7 +881,7 @@ static void x11_send_init_error(struct X11Connection *xconn, reply[0] = 0; /* failure */ reply[1] = msglen; /* length of reason string */ memcpy(reply + 2, xconn->firstpkt + 2, 4); /* major/minor proto vsn */ - PUT_16BIT(xconn->firstpkt[0], reply + 6, msgsize >> 2);/* data len */ + PUT_16BIT_X11(xconn->firstpkt[0], reply + 6, msgsize >> 2);/* data len */ memset(reply + 8, 0, msgsize); memcpy(reply + 8, full_message, msglen); sshfwd_write(xconn->c, reply, 8 + msgsize); @@ -930,8 +930,9 @@ static int x11_send(Channel *chan, bool is_stderr, const void *vdata, int len) * strings, do so now. */ if (!xconn->auth_protocol) { - xconn->auth_plen = GET_16BIT(xconn->firstpkt[0], xconn->firstpkt + 6); - xconn->auth_dlen = GET_16BIT(xconn->firstpkt[0], xconn->firstpkt + 8); + char endian = xconn->firstpkt[0]; + xconn->auth_plen = GET_16BIT_X11(endian, xconn->firstpkt + 6); + xconn->auth_dlen = GET_16BIT_X11(endian, xconn->firstpkt + 8); xconn->auth_psize = (xconn->auth_plen + 3) & ~3; xconn->auth_dsize = (xconn->auth_dlen + 3) & ~3; /* Leave room for a terminating zero, to make our lives easier. */ @@ -967,9 +968,10 @@ static int x11_send(Channel *chan, bool is_stderr, const void *vdata, int len) int socketdatalen; char new_peer_addr[32]; int new_peer_port; + char endian = xconn->firstpkt[0]; - protomajor = GET_16BIT(xconn->firstpkt[0], xconn->firstpkt + 2); - protominor = GET_16BIT(xconn->firstpkt[0], xconn->firstpkt + 4); + protomajor = GET_16BIT_X11(endian, xconn->firstpkt + 2); + protominor = GET_16BIT_X11(endian, xconn->firstpkt + 4); assert(!xconn->s); @@ -1177,10 +1179,10 @@ void *x11_make_greeting(int endian, int protomajor, int protominor, greeting = snewn(greeting_len, unsigned char); memset(greeting, 0, greeting_len); greeting[0] = endian; - PUT_16BIT(endian, greeting+2, protomajor); - PUT_16BIT(endian, greeting+4, protominor); - PUT_16BIT(endian, greeting+6, authnamelen); - PUT_16BIT(endian, greeting+8, authdatalen); + PUT_16BIT_X11(endian, greeting+2, protomajor); + PUT_16BIT_X11(endian, greeting+4, protominor); + PUT_16BIT_X11(endian, greeting+6, authnamelen); + PUT_16BIT_X11(endian, greeting+8, authdatalen); memcpy(greeting+12, authname, authnamelen); memcpy(greeting+12+authnamelen_pad, authdata, authdatalen);