Fix for bug 62159 (support larger port numbers in URL).

[gohanman]

Changes "port" in php_url struct from an unsigned short to a uin32_t so value can be stored. Given how the field is used this appears to be safe to change.

https://bugs.php.net/bug.php?id=62159

[cmb69]

port is declared as long, which only guarentees that it has at least 32 bits, so the code doesn't appear to be portable. That affects not only 32bit systems, but also LLP64 (e.g. Windows).

[Fleshgrinder]

You refer to the port member of the php_url struct and the explicit cast, right? Should be a zend_long.

Also note that 0 is actually an official valid port number.

[cmb69]

You refer to the port member of the php_url struct and the explicit cast, right?

No, I meant that port <= 4294967295 will always be true, if long has only 32bits (CMIIW).

Should be a zend_long.

zend_long is available as of PHP 7.0 only, but the PR targets 5.6.

[Fleshgrinder]

No, I meant that port <= 4294967295 will always be true, if long has only 32bits (CMIIW).

Yes, it has to be and you are absolutely right. No upper bound at all is definitely the better approach here and 0 inclusive as lower bound.

zend_long is available as of PHP 7.0 only, but the PR targets 5.6.

Missed that it targets 5.6. I guess this requires another PR against 7 then where it uses zend_long and #ifdef here to determine if it is 32 or 64 bit.

[nikic]

This can't go into PHP 5.6 anyway, as it breaks the ABI (at least I think so -- the change of the port member size is absorbed by padding, but the values might not be consistent depending on machine endianness. It might be fine on x86, but will break on big endian machines.)

[Fleshgrinder]

Definitely possible, yes.

typedef struct php_url {
    char *scheme;        // 1 byte
    char *user;          // 1 byte
    char *pass;          // 1 byte
    char *host;          // 1 byte
    unsigned short port; // 2 byte
    char *path;          // 1 byte
    char *query;         // 1 byte
    char *fragment;      // 1 byte
    char PADDING[3];     // 3 byte
} php_url;
// 12 byte % 4 = 0

typedef struct php_url {
    char *scheme;    // 1 byte
    char *user;      // 1 byte
    char *pass;      // 1 byte
    char *host;      // 1 byte
    uint32_t port;   // 4 byte
    char *path;      // 1 byte
    char *query;     // 1 byte
    char *fragment;  // 1 byte
    char PADDING[1]; // 1 byte
} php_url;
// 12 byte % 4 = 0

typedef struct php_url {
    char *scheme;    // 1 byte
    char *user;      // 1 byte
    char *pass;      // 1 byte
    char *host;      // 1 byte
    uint64_t port;   // 8 byte
    char *path;      // 1 byte
    char *query;     // 1 byte
    char *fragment;  // 1 byte
    char PADDING[1]; // 1 byte
} php_url;
// 16 byte % 8 = 0

[bukka]

I think this is an ABI break anyway. It's not just about the struct size but also about the offset of the path, query and fragment that would change here. It would be fine if port was the last but it's not the case so I don't think this should go to 5.6 or 7.0. It should go only to 7.1 if anything.

[Fleshgrinder]

@gohanman: will you port this for 7.1?

[gohanman]

@Fleshgrinder I have to get caught up on the comments here first but I'll give it a shot.

[krakjoe]

Since this has merge conflicts, targets a security fix only branch, and seemed unsatisfactory anyway, I'm closing this PR.

Please take this action as encouragement to create a clean PR, targeting a supported (appropriate) branch.

[nikic]

For the record, I think the basic change is fine -- we don't need to be pedantic about the port range. But it needs to go into master due to ABI stability and that range check should probably use something like INT32_MAX (I'd suggest that instead of UINT32_MAX to ensure 32bit and 64bit PHP builds both behave identically).