Opened 7 weeks ago

Closed 7 weeks ago

Last modified 7 weeks ago

#24480 closed defect (fixed)

Signed/unsigned mismatch

Reported by: ln5 Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

rendservice.[ch] are mixing signed and unsigned types for struct rend_intro_cell_s member ciphertext_len.

Child Tickets

Change History (7)

comment:1 Changed 7 weeks ago by ln5

Using clang 3.4.1:

  CC       src/or/rendservice.o
/usr/local/src/tor/src/or/rendservice.c:2027:29: error: comparison of integers of different signs: 'ssize_t'
      (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
    parsed_req->ciphertext, MIN(parsed_req->ciphertext_len, keylen),
                            ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~  ~~~~~~
/usr/include/sys/param.h:298:23: note: expanded from macro 'MIN'
#define MIN(a,b) (((a)<(b))?(a):(b))

comment:2 Changed 7 weeks ago by ln5

See branch bug24480 in my public repo for a fix.

comment:3 Changed 7 weeks ago by ln5

Status: newneeds_review

comment:4 Changed 7 weeks ago by nickm

Bug confirmed.

For the fix, how about we just change keylen to ssize_t instead? It is a smaller and much less risky change, since it's only a local variable, and only used in one place.

Example patch:

diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index ba8891eade39ea..80e1e10a054b4a 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -1162,7 +1162,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request,
   time_t now = time(NULL);
   time_t elapsed;
   int replay;
-  size_t keylen;
+  ssize_t keylen;
 
   /* Do some initial validation and logging before we parse the cell */
   if (circuit->base_.purpose != CIRCUIT_PURPOSE_S_INTRO) {

What do you think?

comment:5 Changed 7 weeks ago by ahf

Status: needs_reviewmerge_ready

Changing size_t keylen to ssize_t keylen looks good to me.

comment:6 Changed 7 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Added a changes file and pushed as 461e34bb3dec0f4f7234584806a224040f44c7ed. Thanks!

comment:7 in reply to:  4 Changed 7 weeks ago by ln5

Replying to nickm:

What do you think?

I agree that's much better. Thanks.

Note: See TracTickets for help on using tickets.