Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5105 closed defect (fixed)

set_managed_proxy_environment should be OS-independent

Reported by: rransom Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

At least two people have been confused by the fact that there are two copies of set_managed_proxy_environment (one for Windows, and one for Unixoids), and that confusion has come close to producing at least two bugs. Time to refactor it to produce a smartlist on all OSes.

Child Tickets

Change History (11)

comment:1 Changed 8 years ago by rransom

Status: newneeds_review

See bug5105-v2 ( https://gitweb.torproject.org/rransom/tor.git bug5105-v2 ) for a not-yet-tested branch that rewrites set_managed_proxy_environment to be OS-independent (and renames it to create_managed_proxy_environment).

The new utility functions should have unit tests.

comment:2 Changed 8 years ago by Sebastian

Status: needs_reviewneeds_revision

This will need changes like we did for #5082

comment:3 Changed 8 years ago by nickm

Looks good modulo #5082 issues (whatever they were) and of course the #5114 changes.

wrt 9101c255914d68801c3a075c2ebfdc4dd267d734:

  • Technically, calloc(0,0) should be legal -- or at the very least, it shouldn't be a divide-by-zero error.

wrt later stuff:

  • Maybe it would be easier to have get_current_process_environment_variables() return a list containing heap-allocated copies of the environment variables? That way we wouldn't need so much bookkeeping to remember what was safe to free and what wasn't, and we could eliminate the possibility of an entire category of error.

In general:

  • Yeah, some unit tests would be great.

comment:4 Changed 8 years ago by rransom

I accidentally fixed a bug in this branch (currently Tor does not override any variables in its environment when setting up the managed proxy's environment), so it will also need a changes/ file.

comment:5 in reply to:  3 ; Changed 8 years ago by rransom

Replying to nickm:

Looks good modulo #5082 issues (whatever they were) and of course the #5114 changes.

wrt 9101c255914d68801c3a075c2ebfdc4dd267d734:

  • Technically, calloc(0,0) should be legal -- or at the very least, it shouldn't be a divide-by-zero error.

Oops.

wrt later stuff:

  • Maybe it would be easier to have get_current_process_environment_variables() return a list containing heap-allocated copies of the environment variables? That way we wouldn't need so much bookkeeping to remember what was safe to free and what wasn't, and we could eliminate the possibility of an entire category of error.

But this way is faster!

I'll make this change the next time I touch this branch, although we might want a non-strdup-ing version of get_current_process_environment_variables someday.

In general:

  • Yeah, some unit tests would be great.

comment:6 in reply to:  5 ; Changed 8 years ago by nickm

Replying to rransom:

Replying to nickm:

  • Technically, calloc(0,0) should be legal -- or at the very least, it shouldn't be a divide-by-zero error.

Oops.

(Actually, to be scrupulous, calloc(0,0) isn't required to be legal because of any C standard, but for consistency with tor_malloc(0).)

wrt later stuff:

  • Maybe it would be easier to have get_current_process_environment_variables() return a list containing heap-allocated copies of the environment variables? That way we wouldn't need so much bookkeeping to remember what was safe to free and what wasn't, and we could eliminate the possibility of an entire category of error.

But this way is faster!

Faster but trickier and more error-prone. If this ever shows up in a profile, we have done something terribly wrong.

Friends don't let friends optimize outside the critical path. ;)

I'll make this change the next time I touch this branch, although we might want a non-strdup-ing version of get_current_process_environment_variables someday.

In general:

  • Yeah, some unit tests would be great.

--

Are you working on changes here, or should somebody else pick up the revisions?

comment:7 in reply to:  6 Changed 8 years ago by rransom

Status: needs_revisionneeds_review

Replying to nickm:

Are you working on changes here, or should somebody else pick up the revisions?

Done and pushed. I'm not convinced that having get_current_process_environment_variables allocate the strings it returns on the heap is an improvement, though.

comment:8 Changed 8 years ago by rransom

I've pushed a few more fixup commits, this time including a changes/ file.

comment:9 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great; squashed and merged. Please check out the merge commit; there was a conflict in transports.c

comment:10 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:11 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.