Opened 7 years ago

Last modified 15 months ago

#4229 new defect

smartlist_len returns a (signed) int

Reported by: rransom Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client refactoring c correctness
Cc: mansourmoufid@… Actual Points:
Parent ID: Points: 10
Reviewer: Sponsor:

Description

smartlist_len(sl) has type int. It shouldn't be signed.

Child Tickets

Attachments (1)

foo.patch (4.1 KB) - added by mansour 7 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by rransom

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

I don't think this can be fixed on 0.2.2.x. (We would have too many merge conflicts.)

comment:2 Changed 7 years ago by rransom

It's also not quite clear what we want to do for lengths in general -- do we sometimes use negative return values to indicate failure of a function that would normally return a length? At the very least, we need safe (overflow-checking) unsigned-to-signed and signed-to-unsigned cast macros.

Changed 7 years ago by mansour

Attachment: foo.patch added

comment:3 in reply to:  2 Changed 7 years ago by mansour

Replying to rransom:

It's also not quite clear what we want to do for lengths in general -- do we sometimes use negative return values to indicate failure of a function that would normally return a length? At the very least, we need safe (overflow-checking) unsigned-to-signed and signed-to-unsigned cast macros.

One approach is to pass a pointer to where "returned" lengths will be stored as an argument, and use the return values to indicate errors only.

See attached for an example.

comment:4 Changed 7 years ago by mansour

Cc: mansourmoufid@… added

comment:5 Changed 7 years ago by nickm

If we're going to change the return type of smartlist_len() {or even if we're not}, we need to be prepared to audit all 448 places that uses it, to make sure it behaves right given an unsigned or a size_t or whatever.

As a stopgap, perhaps we should just ensure that a smartlist's size can't grow larger than INT_MAX ?

comment:6 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

At the moment, actually, thanks to the smartlist_ensure_capacity fixes, we have the property that the capacity of a smartlist can never be more than INT_MAX . So it's okay to return an int for smartlist_len. I'm throwing this out of 0.2.3.x; it's a big refactoring, and I'm pretty sure that we don't actually *want* to allow a smartlist to have anything close to INT_MAX entries.

comment:7 Changed 6 years ago by nickm

Keywords: tor-client added

comment:8 Changed 6 years ago by nickm

Component: Tor ClientTor

comment:9 Changed 15 months ago by nickm

Keywords: refactoring c correctness added
Points: 10
Severity: Normal
Note: See TracTickets for help on using tickets.