{"action":"created","comment":{"url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/comments/178831425","pull_request_review_id":108956231,"id":178831425,"diff_hunk":"@@ -49,8 +61,8 @@ let from_shadow_t_opt = function\n \n let to_shadow_t sp =\n   let sp_t : shadow_t structure = make shadow_t in\n-  setf sp_t sp_name sp.name;\n-  setf sp_t sp_passwd sp.passwd;\n+  setf sp_t sp_name (sp.name |> string_to_char_array |> CArray.start);\n+  setf sp_t sp_passwd (sp.passwd |> string_to_char_array |> CArray.start);","path":"lib/shadow.ml","position":42,"original_position":42,"commit_id":"e1a63010ae3b840c4c3596600ed3d25a633e0af8","original_commit_id":"e1a63010ae3b840c4c3596600ed3d25a633e0af8","user":{"login":"edwintorok","id":721894,"avatar_url":"https://avatars1.githubusercontent.com/u/721894?v=4","gravatar_id":"","url":"https://api.github.com/users/edwintorok","html_url":"https://github.com/edwintorok","followers_url":"https://api.github.com/users/edwintorok/followers","following_url":"https://api.github.com/users/edwintorok/following{/other_user}","gists_url":"https://api.github.com/users/edwintorok/gists{/gist_id}","starred_url":"https://api.github.com/users/edwintorok/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/edwintorok/subscriptions","organizations_url":"https://api.github.com/users/edwintorok/orgs","repos_url":"https://api.github.com/users/edwintorok/repos","events_url":"https://api.github.com/users/edwintorok/events{/privacy}","received_events_url":"https://api.github.com/users/edwintorok/received_events","type":"User","site_admin":false},"body":"Shouldn't we store the CArray handles somewhere to make sure they are not garbage collected? I'm not sure whether between here and the actual call there would be anything to keep the carray reference live:\r\n`putspent' (to_shadow_t sp |> addr) fd |> ignore`\r\nI think due to the way how CArrays are implemented this would work (`start` gives you the `astart` field, which gets stored in the struct, and taking the address of the struct hopefully keeps the struct itself alive too?).\r\n\r\nIf we want more control over lifetimes we might have to inline `to_shadow_t`, use `let sp_name_carray = ... in ... putspent`. The pipe operator is nice, but the lifetime of its input might stop too early.\r\nTo be sure maybe we could have a `let gc_id x = Gc.compact (); x` and put calls to `gc_id` on each pipe operator? That might allow us to reproduce the original problem more reliably.","created_at":"2018-04-03T13:53:04Z","updated_at":"2018-04-03T13:54:18Z","html_url":"https://github.com/xapi-project/ocaml-opasswd/pull/11#discussion_r178831425","pull_request_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11","author_association":"NONE","_links":{"self":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/comments/178831425"},"html":{"href":"https://github.com/xapi-project/ocaml-opasswd/pull/11#discussion_r178831425"},"pull_request":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11"}}},"pull_request":{"url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11","id":179094413,"html_url":"https://github.com/xapi-project/ocaml-opasswd/pull/11","diff_url":"https://github.com/xapi-project/ocaml-opasswd/pull/11.diff","patch_url":"https://github.com/xapi-project/ocaml-opasswd/pull/11.patch","issue_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/11","number":11,"state":"open","locked":false,"title":"CA-281402: Update string fields to char ptrs","user":{"login":"mseri","id":4040623,"avatar_url":"https://avatars0.githubusercontent.com/u/4040623?v=4","gravatar_id":"","url":"https://api.github.com/users/mseri","html_url":"https://github.com/mseri","followers_url":"https://api.github.com/users/mseri/followers","following_url":"https://api.github.com/users/mseri/following{/other_user}","gists_url":"https://api.github.com/users/mseri/gists{/gist_id}","starred_url":"https://api.github.com/users/mseri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mseri/subscriptions","organizations_url":"https://api.github.com/users/mseri/orgs","repos_url":"https://api.github.com/users/mseri/repos","events_url":"https://api.github.com/users/mseri/events{/privacy}","received_events_url":"https://api.github.com/users/mseri/received_events","type":"User","site_admin":false},"body":"From the mailing list discussion:\r\n\r\n> When the memory is owned by C code -- for example, when a C function\r\n  passes a struct to OCaml with fields already initialized -- then using\r\n  the string view works well: reading a string field creates an OCaml\r\n  copy, which persists as long as needed.\r\n\r\n> However, when things are the other way round -- i.e. when you're\r\n  creating or initializing the struct in OCaml before passing it to C --\r\n  then the string view isn't a good choice, because there's no way to\r\n  manage the lifetime of the C copy of the OCaml string that's written\r\n  to the struct field.\r\n\r\n> A reasonable way to do this would be to allocate stable memory for the\r\n  string fields before the C call, and ensure that the memory stays\r\n  around until the call is complete.  Stable memory could mean any of: a\r\n  bigarray, a Ctypes.CArray.t value, memory returned by malloc, memory\r\n  returned by Ctypes.allocate, etc.  In most of these cases ensuring the\r\n  memory stays around is a matter of holding onto the handle (bigarray\r\n  value, Ctypes ptr, etc.) until after the call.\r\n\r\nThis PR is to remove the string view and use the char ptrs instead.\r\nThe tricky parts are extracting the string as we don't know the lenght,\r\nand making them live long enough (but I think that was already ok).\r\n\r\nInitially I did the following\r\n\r\n```ocaml\r\n(* We can't define it as\r\n   CArray.(string_from_ptr (start a) (length a))\r\n   as we do not know the length of the string a priori.\r\n\r\n   Will fail with `Invalid_argument \"index_out_of_bounds\" if\r\n   we don't reach the null terminator by [maxlen] chars.\r\n*)\r\nlet char_array_as_string ?(maxlen=1024) a =\r\n  (* here we overflow but we should never read after the string if the\r\n     memory is not corrupted *)\r\n  let a': char carray = CArray.from_ptr a maxlen in\r\n  let b = Buffer.create 128 in\r\n  let rec scan idx =\r\n    let c = CArray.get a' idx in\r\n    if c = '\\x00' then\r\n      Buffer.contents b\r\n    else begin\r\n      Buffer.add_char b c;\r\n      scan (idx+1)\r\n    end\r\n  in\r\n  scan 0\r\n```\r\n\r\nBut I think that the above is morally equivalent to the current\r\n\r\n```ocaml\r\nlet ptr_char_to_string p =\r\n  let s = coerce (ptr char) string p in\r\n  let b = Buffer.create 128 in\r\n  Buffer.add_string b s;\r\n  Buffer.contents b\r\n```\r\n\r\nalthough more explicit. In both cases we risk to do an overflow and\r\nfail. It could be that the original is better as we fail more explicitly\r\nwhile here we are somewhat hoping for the best. Either way, we are\r\nstrongly relying on the C strings to be null terminated and the memory\r\nto be sane.\r\n\r\nSigned-off-by: Marcello Seri <marcello.seri@citrix.com>","created_at":"2018-04-03T13:13:39Z","updated_at":"2018-04-03T13:54:18Z","closed_at":null,"merged_at":null,"merge_commit_sha":"8e79515d453c6b65c7cc644532e4ff7b2752dc33","assignee":null,"assignees":[],"requested_reviewers":[{"login":"jonludlam","id":210963,"avatar_url":"https://avatars3.githubusercontent.com/u/210963?v=4","gravatar_id":"","url":"https://api.github.com/users/jonludlam","html_url":"https://github.com/jonludlam","followers_url":"https://api.github.com/users/jonludlam/followers","following_url":"https://api.github.com/users/jonludlam/following{/other_user}","gists_url":"https://api.github.com/users/jonludlam/gists{/gist_id}","starred_url":"https://api.github.com/users/jonludlam/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonludlam/subscriptions","organizations_url":"https://api.github.com/users/jonludlam/orgs","repos_url":"https://api.github.com/users/jonludlam/repos","events_url":"https://api.github.com/users/jonludlam/events{/privacy}","received_events_url":"https://api.github.com/users/jonludlam/received_events","type":"User","site_admin":false}],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11/commits","review_comments_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11/comments","review_comment_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/comments{/number}","comments_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/11/comments","statuses_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/statuses/e1a63010ae3b840c4c3596600ed3d25a633e0af8","head":{"label":"mseri:CA-281402","ref":"CA-281402","sha":"e1a63010ae3b840c4c3596600ed3d25a633e0af8","user":{"login":"mseri","id":4040623,"avatar_url":"https://avatars0.githubusercontent.com/u/4040623?v=4","gravatar_id":"","url":"https://api.github.com/users/mseri","html_url":"https://github.com/mseri","followers_url":"https://api.github.com/users/mseri/followers","following_url":"https://api.github.com/users/mseri/following{/other_user}","gists_url":"https://api.github.com/users/mseri/gists{/gist_id}","starred_url":"https://api.github.com/users/mseri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mseri/subscriptions","organizations_url":"https://api.github.com/users/mseri/orgs","repos_url":"https://api.github.com/users/mseri/repos","events_url":"https://api.github.com/users/mseri/events{/privacy}","received_events_url":"https://api.github.com/users/mseri/received_events","type":"User","site_admin":false},"repo":{"id":101062527,"name":"ocaml-opasswd","full_name":"mseri/ocaml-opasswd","owner":{"login":"mseri","id":4040623,"avatar_url":"https://avatars0.githubusercontent.com/u/4040623?v=4","gravatar_id":"","url":"https://api.github.com/users/mseri","html_url":"https://github.com/mseri","followers_url":"https://api.github.com/users/mseri/followers","following_url":"https://api.github.com/users/mseri/following{/other_user}","gists_url":"https://api.github.com/users/mseri/gists{/gist_id}","starred_url":"https://api.github.com/users/mseri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mseri/subscriptions","organizations_url":"https://api.github.com/users/mseri/orgs","repos_url":"https://api.github.com/users/mseri/repos","events_url":"https://api.github.com/users/mseri/events{/privacy}","received_events_url":"https://api.github.com/users/mseri/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/mseri/ocaml-opasswd","description":"OCaml bindings for glibc's passwd and shadow password file interface","fork":true,"url":"https://api.github.com/repos/mseri/ocaml-opasswd","forks_url":"https://api.github.com/repos/mseri/ocaml-opasswd/forks","keys_url":"https://api.github.com/repos/mseri/ocaml-opasswd/keys{/key_id}","collaborators_url":"https://api.github.com/repos/mseri/ocaml-opasswd/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/mseri/ocaml-opasswd/teams","hooks_url":"https://api.github.com/repos/mseri/ocaml-opasswd/hooks","issue_events_url":"https://api.github.com/repos/mseri/ocaml-opasswd/issues/events{/number}","events_url":"https://api.github.com/repos/mseri/ocaml-opasswd/events","assignees_url":"https://api.github.com/repos/mseri/ocaml-opasswd/assignees{/user}","branches_url":"https://api.github.com/repos/mseri/ocaml-opasswd/branches{/branch}","tags_url":"https://api.github.com/repos/mseri/ocaml-opasswd/tags","blobs_url":"https://api.github.com/repos/mseri/ocaml-opasswd/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/mseri/ocaml-opasswd/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/mseri/ocaml-opasswd/git/refs{/sha}","trees_url":"https://api.github.com/repos/mseri/ocaml-opasswd/git/trees{/sha}","statuses_url":"https://api.github.com/repos/mseri/ocaml-opasswd/statuses/{sha}","languages_url":"https://api.github.com/repos/mseri/ocaml-opasswd/languages","stargazers_url":"https://api.github.com/repos/mseri/ocaml-opasswd/stargazers","contributors_url":"https://api.github.com/repos/mseri/ocaml-opasswd/contributors","subscribers_url":"https://api.github.com/repos/mseri/ocaml-opasswd/subscribers","subscription_url":"https://api.github.com/repos/mseri/ocaml-opasswd/subscription","commits_url":"https://api.github.com/repos/mseri/ocaml-opasswd/commits{/sha}","git_commits_url":"https://api.github.com/repos/mseri/ocaml-opasswd/git/commits{/sha}","comments_url":"https://api.github.com/repos/mseri/ocaml-opasswd/comments{/number}","issue_comment_url":"https://api.github.com/repos/mseri/ocaml-opasswd/issues/comments{/number}","contents_url":"https://api.github.com/repos/mseri/ocaml-opasswd/contents/{+path}","compare_url":"https://api.github.com/repos/mseri/ocaml-opasswd/compare/{base}...{head}","merges_url":"https://api.github.com/repos/mseri/ocaml-opasswd/merges","archive_url":"https://api.github.com/repos/mseri/ocaml-opasswd/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/mseri/ocaml-opasswd/downloads","issues_url":"https://api.github.com/repos/mseri/ocaml-opasswd/issues{/number}","pulls_url":"https://api.github.com/repos/mseri/ocaml-opasswd/pulls{/number}","milestones_url":"https://api.github.com/repos/mseri/ocaml-opasswd/milestones{/number}","notifications_url":"https://api.github.com/repos/mseri/ocaml-opasswd/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/mseri/ocaml-opasswd/labels{/name}","releases_url":"https://api.github.com/repos/mseri/ocaml-opasswd/releases{/id}","deployments_url":"https://api.github.com/repos/mseri/ocaml-opasswd/deployments","created_at":"2017-08-22T12:53:17Z","updated_at":"2017-08-22T12:53:18Z","pushed_at":"2018-04-03T13:16:46Z","git_url":"git://github.com/mseri/ocaml-opasswd.git","ssh_url":"git@github.com:mseri/ocaml-opasswd.git","clone_url":"https://github.com/mseri/ocaml-opasswd.git","svn_url":"https://github.com/mseri/ocaml-opasswd","homepage":null,"size":96,"stargazers_count":0,"watchers_count":0,"language":"OCaml","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":null,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"xapi-project:master","ref":"master","sha":"056c6cac676f9ef21ef884f8c8d7f88b14c44b72","user":{"login":"xapi-project","id":317268,"avatar_url":"https://avatars3.githubusercontent.com/u/317268?v=4","gravatar_id":"","url":"https://api.github.com/users/xapi-project","html_url":"https://github.com/xapi-project","followers_url":"https://api.github.com/users/xapi-project/followers","following_url":"https://api.github.com/users/xapi-project/following{/other_user}","gists_url":"https://api.github.com/users/xapi-project/gists{/gist_id}","starred_url":"https://api.github.com/users/xapi-project/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xapi-project/subscriptions","organizations_url":"https://api.github.com/users/xapi-project/orgs","repos_url":"https://api.github.com/users/xapi-project/repos","events_url":"https://api.github.com/users/xapi-project/events{/privacy}","received_events_url":"https://api.github.com/users/xapi-project/received_events","type":"Organization","site_admin":false},"repo":{"id":14028451,"name":"ocaml-opasswd","full_name":"xapi-project/ocaml-opasswd","owner":{"login":"xapi-project","id":317268,"avatar_url":"https://avatars3.githubusercontent.com/u/317268?v=4","gravatar_id":"","url":"https://api.github.com/users/xapi-project","html_url":"https://github.com/xapi-project","followers_url":"https://api.github.com/users/xapi-project/followers","following_url":"https://api.github.com/users/xapi-project/following{/other_user}","gists_url":"https://api.github.com/users/xapi-project/gists{/gist_id}","starred_url":"https://api.github.com/users/xapi-project/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/xapi-project/subscriptions","organizations_url":"https://api.github.com/users/xapi-project/orgs","repos_url":"https://api.github.com/users/xapi-project/repos","events_url":"https://api.github.com/users/xapi-project/events{/privacy}","received_events_url":"https://api.github.com/users/xapi-project/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/xapi-project/ocaml-opasswd","description":"OCaml bindings for glibc's passwd and shadow password file interface","fork":false,"url":"https://api.github.com/repos/xapi-project/ocaml-opasswd","forks_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/forks","keys_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/keys{/key_id}","collaborators_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/teams","hooks_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/hooks","issue_events_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/events{/number}","events_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/events","assignees_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/assignees{/user}","branches_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/branches{/branch}","tags_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/tags","blobs_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/git/refs{/sha}","trees_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/git/trees{/sha}","statuses_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/statuses/{sha}","languages_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/languages","stargazers_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/stargazers","contributors_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/contributors","subscribers_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/subscribers","subscription_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/subscription","commits_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/commits{/sha}","git_commits_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/git/commits{/sha}","comments_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/comments{/number}","issue_comment_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/comments{/number}","contents_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/contents/{+path}","compare_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/compare/{base}...{head}","merges_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/merges","archive_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/downloads","issues_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues{/number}","pulls_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls{/number}","milestones_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/milestones{/number}","notifications_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/labels{/name}","releases_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/releases{/id}","deployments_url":"https://api.github.com/repos/xapi-project/ocaml-opasswd/deployments","created_at":"2013-10-31T21:11:25Z","updated_at":"2017-08-04T15:52:21Z","pushed_at":"2018-04-03T13:16:48Z","git_url":"git://github.com/xapi-project/ocaml-opasswd.git","ssh_url":"git@github.com:xapi-project/ocaml-opasswd.git","clone_url":"https://github.com/xapi-project/ocaml-opasswd.git","svn_url":"https://github.com/xapi-project/ocaml-opasswd","homepage":null,"size":95,"stargazers_count":3,"watchers_count":3,"language":"OCaml","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":6,"mirror_url":null,"archived":false,"open_issues_count":1,"license":{"key":"isc","name":"ISC License","spdx_id":"ISC","url":"https://api.github.com/licenses/isc"},"forks":6,"open_issues":1,"watchers":3,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11"},"html":{"href":"https://github.com/xapi-project/ocaml-opasswd/pull/11"},"issue":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/11"},"comments":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/issues/11/comments"},"review_comments":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11/comments"},"review_comment":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/pulls/11/commits"},"statuses":{"href":"https://api.github.com/repos/xapi-project/ocaml-opasswd/statuses/e1a63010ae3b840c4c3596600ed3d25a633e0af8"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"edwintorok"}}