{"action":"created","issue":{"url":"https://api.github.com/repos/openssl/openssl/issues/3877","repository_url":"https://api.github.com/repos/openssl/openssl","labels_url":"https://api.github.com/repos/openssl/openssl/issues/3877/labels{/name}","comments_url":"https://api.github.com/repos/openssl/openssl/issues/3877/comments","events_url":"https://api.github.com/repos/openssl/openssl/issues/3877/events","html_url":"https://github.com/openssl/openssl/issues/3877","id":241114153,"number":3877,"title":"Memory bounds checking in x_long.c","user":{"login":"paulidale","id":17916609,"avatar_url":"https://avatars1.githubusercontent.com/u/17916609?v=4","gravatar_id":"","url":"https://api.github.com/users/paulidale","html_url":"https://github.com/paulidale","followers_url":"https://api.github.com/users/paulidale/followers","following_url":"https://api.github.com/users/paulidale/following{/other_user}","gists_url":"https://api.github.com/users/paulidale/gists{/gist_id}","starred_url":"https://api.github.com/users/paulidale/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/paulidale/subscriptions","organizations_url":"https://api.github.com/users/paulidale/orgs","repos_url":"https://api.github.com/users/paulidale/repos","events_url":"https://api.github.com/users/paulidale/events{/privacy}","received_events_url":"https://api.github.com/users/paulidale/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/openssl/openssl/milestones/9","html_url":"https://github.com/openssl/openssl/milestone/9","labels_url":"https://api.github.com/repos/openssl/openssl/milestones/9/labels","id":2145708,"number":9,"title":"1.1.1","description":"","creator":{"login":"levitte","id":698918,"avatar_url":"https://avatars1.githubusercontent.com/u/698918?v=4","gravatar_id":"","url":"https://api.github.com/users/levitte","html_url":"https://github.com/levitte","followers_url":"https://api.github.com/users/levitte/followers","following_url":"https://api.github.com/users/levitte/following{/other_user}","gists_url":"https://api.github.com/users/levitte/gists{/gist_id}","starred_url":"https://api.github.com/users/levitte/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/levitte/subscriptions","organizations_url":"https://api.github.com/users/levitte/orgs","repos_url":"https://api.github.com/users/levitte/repos","events_url":"https://api.github.com/users/levitte/events{/privacy}","received_events_url":"https://api.github.com/users/levitte/received_events","type":"User","site_admin":false},"open_issues":128,"closed_issues":236,"state":"open","created_at":"2016-11-19T09:18:52Z","updated_at":"2018-04-25T12:59:36Z","due_on":null,"closed_at":null},"comments":55,"created_at":"2017-07-06T23:22:31Z","updated_at":"2018-04-26T07:35:58Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Rather than discuss this in the PR, I thought it worth making an issue of it since it will need addressing.\r\n\r\nThe file in question is `crypto/asn1/x_long.c`  The relevant commit is [here](https://github.com/openssl/openssl/commit/60eba30f60de55e3c782469fa555eede82606099).\r\n\r\nOver in #3869 @dot-asm wrote:\r\n\r\n> H-m-m-m-m... I feel that I have to rise kind of objection to [at least] rationale behind replacing *(long *)x = y with memcpy. I mean rationale is to counter undefined behaviour. Well, formally speaking memcpy doesn't really cut it. The change kind of suggests that we are working with kind of \"wire\" data, i.e. pieces of data payload that is or is to be delivered by some inter-peer communication means. But[!] if this was actually case, then we wouldn't be able to assume that data is arranged in host byte order. I mean surrounding code is written under assumption that data comes in host byte order, isn't it? And consequently we have to assume that producer or consumer of this data is application code in same process context, not some \"wire\" data. But in such case behaviour is rendered undefined only if original and assumed types are different. Which in turn means that memcpy would be as inappropriate. [And that COPY_SIZE thing actually makes it apparent, doesn't it?]\r\nJust in case for reference. This kind of contradicts concerns I've risen in #3701. Well, yes, but there is difference between first impressions/reactions and conclusions you can draw after actually contemplating the problem. This is such case...\r\n\r\nCan we agree that `*(long *)x = y` is bad?  In these cases, it is undefined according to the C standard -- it is allowed to do nothing, crash the program or format the boot drive although it probably tries to do something resembling what is asked.  There are alignment and size concerns when the alignment of long is stricter than that of a pointer or when a long is larger than a pointer.\r\n\r\nThe `memcpy` solution is better in the sense that it is well defined and the COPY_SIZE _thing_ (good word, it's ugly) prevents overflows.  I think there could be problems with loss of information if the size of a pointer and a long don't match.  Is `memcpy` likely to be confused with a \"wire\" operation in this context?  There are no endian guards which indicate \"wire\" more strong to me.\r\n\r\nI don't think a union will be any better.  A size mismatch will could result in loss of information, although it would cover the overflow concerns.\r\n\r\nWould using a `ptrint_t` instead of a `long` make sense?  It doesn't feel right to me.\r\n\r\nWould allocating a block on the heap to contain the `long` make sense?  This seems like the proper solution.  The pointer stays a pointer and overflows aren't possible.  Are memory leaks likely to result?\r\n"},"comment":{"url":"https://api.github.com/repos/openssl/openssl/issues/comments/384543430","html_url":"https://github.com/openssl/openssl/issues/3877#issuecomment-384543430","issue_url":"https://api.github.com/repos/openssl/openssl/issues/3877","id":384543430,"user":{"login":"levitte","id":698918,"avatar_url":"https://avatars1.githubusercontent.com/u/698918?v=4","gravatar_id":"","url":"https://api.github.com/users/levitte","html_url":"https://github.com/levitte","followers_url":"https://api.github.com/users/levitte/followers","following_url":"https://api.github.com/users/levitte/following{/other_user}","gists_url":"https://api.github.com/users/levitte/gists{/gist_id}","starred_url":"https://api.github.com/users/levitte/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/levitte/subscriptions","organizations_url":"https://api.github.com/users/levitte/orgs","repos_url":"https://api.github.com/users/levitte/repos","events_url":"https://api.github.com/users/levitte/events{/privacy}","received_events_url":"https://api.github.com/users/levitte/received_events","type":"User","site_admin":false},"created_at":"2018-04-26T07:35:58Z","updated_at":"2018-04-26T07:35:58Z","author_association":"OWNER","body":"This seems to have died off.  As a final note, the use of LONG / ZLONG is *strongly discouraged* (and as mentioned before, to be deprecated), so I don't see that there's much benefit in fixing them.\r\n\r\nUnless someone cries bloody murder, I'll close this issue next time around that I see it."}}
{"actor":{"display_login":"levitte"}}