Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3571 closed enhancement (fixed)

obfsproxy: Please review portability fixes

Reported by: zwol Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: asn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

As a way of getting familiar with the code, I've done a bunch of portability fixes and cleanups to obfsproxy. Please review https://github.com/zackw/obfsproxy-port-fixes/compare/master...port-fixes and pull if you like 'em.

Child Tickets

Change History (7)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

comment:2 Changed 8 years ago by asn

I'm not one to review portability fixes; my automake/portability skills are non-existent, so I just read the commits that I have some experience with.

I checked out "Avoid embedding struct sockaddr in protocol_params_t." which is a thing I wanted to do for a while and it seems okay. When my function documentation branch (#3301) gets merged, we should mention that addr_out should be freed on resolve_address_port()'s documentation.

I checked out "Clean up the logging API." which also seems okay in my book, apart from the fact that the function documentation is now outdated, left with the "-1/1" return convention.

I also think that "Move obfs2_crypt.[ch] to top level and rename them crypt.[ch]." is a good idea (at least till a better idea comes up).

comment:3 Changed 8 years ago by zwol

I renamed the github repository after realizing that it was silly to create N of them for every patch series I generate. The review URL is now https://github.com/zackw/obfsproxy/compare/master...port-fixes

comment:4 Changed 8 years ago by nickm

Looking through this now.

BTW, a request for future patches: please have each commit do only one thing. In particular, it's okay to fix formatting and whitespace, but please do it as a separate commit so that it's easier to review the real changes.

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged, with many conflicts.

Please have a close look through c37354b1e0adcb6d16929613e3bd4a509d487d2d to make sure I didn't screw anything up.

comment:6 Changed 8 years ago by zwol

I read carefully through both sides of the merge and didn't see any problems. Also, I've verified that the result still builds and passes unit tests on Linux and Windows.

I did notice a new portability problem introduced on the other side of the merge (specifically, with the change that introduced doubly linked lists).

{{{#define OFFSETOF(container_type, element) \

(((char*)&((container_type*)0)->element) - ((char*) ((container_type*)0)))}}}

This construct provokes undefined behavior, as it dereferences the null pointer (yes, even though that dereference is inside the argument of the address-of operator). Sufficiently aggressive C optimizers can and will break it. (No version of GCC that I have convenient access to actually does break it, but I recall its fragility being discussed on the GCC mailing lists, and I wouldn't be surprised if clang had a problem with it.) It's also completely unnecessary. C89 provides offsetof, with identical effect, in <stddef.h>; the last environment I'm aware of that didn't have <stddef.h> was the K&R-only compiler bundled with some versions of Solaris -- but obfsproxy won't compile with that antique anyway.

I'll send in another patch to get rid of it.

comment:7 Changed 8 years ago by arma

Component: Pluggable transportObfsproxy
Note: See TracTickets for help on using tickets.