Opened 8 years ago

Closed 8 years ago

#7304 closed enhancement (fixed)

tor_snprintf instead of snprintf

Reported by: ultramage Owned by:
Priority: Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #7754 Points:
Reviewer: Sponsor:

Description

Several places call 'snprintf' from stdio.h instead of 'tor_snprintf' from common/compat.h. Replacing them removes one dependency that complicates Windows nmake builds.

Complete list of occurences:
src/common/util.c (<= 0.2.3.24-rc)
src/or/routerparse.c
src/ext/eventdns.c
src/ext/OpenBSD_malloc_Linux.c
src/ext/tinytest.c

Also, since the Microsoft _snprintf function has nonstandard semantics (which is why they renamed it), doing "#define snprintf _snprintf", as in src/or/or.h and src/common/compat.h is okay, but not so great.

Similarly, 'vsnprintf' could be replaced by 'tor_vsnprintf' in two places, which would simplify the #ifdefing going on.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.4.x-final
Priority: trivialminor

Sounds good to me. If this is a code-quality issue rather than actually causing a bug, it should indeed be in 0.2.4.

If you (or anybody) can write a patch for this against master, I'd appreciate that. A couple of things to keep in mind: The src/ext/* stuff is externally-maintained code, so any changes made to it need to be noninvasive. That would mean:

  • doing "#define snprintf tor_snprintf" or something in a new tinytest_local.h file and adding -DHAVE_TINYTEST_LOCAL to the build flags for tinytest.c
  • doing "#define snprintf tor_snprintf" in eventdns_tor.h, which is where we keep noninvasive changes for that module.
  • For OpenBSD_malloc_Linux.c, I have no idea. I'd be a little surprised if that module built for Windows at all!

It would also be neat to have a way to make a list of forbidden functions to make sure that no place in Tor (except for a couple of specific wrappers) were calling them, so that nobody adds such calls back in the future.

comment:2 Changed 8 years ago by ultramage

The strange "undefined reference to _snprintf" dependency error I couldn't satisfy is gone in 0.2.4, so yeah consider it just a code quality issue.

comment:3 Changed 8 years ago by nickm

Parent ID: #7754

comment:4 Changed 8 years ago by nickm

Added a possible fix for this to 024_msvc as db4810613006f53a72675f32be4a615446248100.

Some files in src/ext still use snprintf, but they shouldn't be ones we build on Windows.

comment:5 Changed 8 years ago by nickm

Status: newneeds_review

comment:6 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged a fix for this into master as of b998431a33db2b.

Note: See TracTickets for help on using tickets.