<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html lang="en">
<head>
<meta content="text/html; charset=US-ASCII" http-equiv="Content-Type">
<title>
GitLab
</title>
<style data-premailer="ignore" type="text/css">
a { color: #1068bf; }
</style>
<style>img {
max-width: 100%; height: auto;
}
body {
font-size: 0.875rem;
}
body {
-webkit-text-shadow: rgba(255,255,255,0.01) 0 0 1px;
}
body {
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Noto Sans", Ubuntu, Cantarell, "Helvetica Neue", sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; font-size: inherit;
}
</style>
</head>
<body style='font-size: inherit; -webkit-text-shadow: rgba(255,255,255,0.01) 0 0 1px; font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Noto Sans", Ubuntu, Cantarell, "Helvetica Neue", sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji";'>
<div class="content">
<p style="color: #777777;">
<a href="https://gitlab.com/support-bot">GitLab Support Bot</a>
<a href="https://gitlab.com/gnutls/gnutls/-/issues/1414#note_1143168550">commented</a>:
</p>
<div class="md" style="color: #303030; word-wrap: break-word;">
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0 0 0.5rem;" align="initial">
<p style="color: inherit; line-height: 1.5; margin: 0 0 16px;">New response for issue <a href="https://gitlab.com/gnutls/gnutls/-/issues/1414" data-reference-type="issue" data-original="#1414" data-link="false" data-link-reference="false" data-project="179611" data-issue="116852658" data-project-path="gnutls/gnutls" data-iid="1414" data-issue-type="issue" data-container="body" data-placement="top" title="bug in gnutls_init()" class="gfm gfm-issue" style="margin-top: 0;">#1414</a>:</p>
<p style="color: inherit; line-height: 1.5; margin: 0 0 16px;">Author: Zoltán Fridrich</p>
<p style="color: inherit; line-height: 1.5; margin: 0;">I believe that if a function outputs its result via an argument, the argument should be left untouched in case of an error. Currently this is clearly not the case with <code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; margin-top: 0; font-weight: inherit; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>gnutls_init</code> which sets session to <code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; font-weight: inherit; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>NULL</code> in some cases (but not all) when error occurs. However, I think that handling an error as follows is generally an incorrect approach.</p>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">But after auditing the code base, over 80% of the gnutls*_init()
functions DO set the parameter to NULL on most or all of the error
cases; only a very limited few were extremely careful to never touch
the parameter on failure (gnutls_x509_crt_init being a prime example).</p>
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0.5rem 0;" align="initial">
<div class="gl-relative markdown-code-block js-markdown-code" style="margin-top: 0;">
<pre class="code highlight js-syntax-highlight language-plaintext" lang="plaintext" data-canonical-lang="" v-pre="true" style='display: block; font-size: 13px; color: #303030; line-height: 1.6em; overflow-x: auto; border-radius: 4px; position: relative; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; word-break: break-all; word-wrap: break-word; background-color: #fafafa; margin: 0 0 16px; padding: 12px; border: 1px solid #dbdbdb;'><code style='font-size: inherit; color: inherit; word-wrap: normal; word-break: keep-all; background-color: inherit; border-radius: 4px; white-space: pre; margin-top: 0; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; overflow-wrap: normal; padding: unset;'><span id="LC1" class="line" lang="plaintext" style="margin-top: 0;">ret = gnutls_init(session, flags);</span>
<span id="LC2" class="line" lang="plaintext">if (ret < 0)</span>
<span id="LC3" class="line" lang="plaintext"> gnutls_deinit(session);</span>
<span id="LC4" class="line" lang="plaintext"> return ret;</span></code></pre>
<copy-code></copy-code>
</div>
<p style="color: inherit; line-height: 1.5; margin: 0;">User should not expect to clean up functions output if that function failed. If this results in segfault, than its the users fault (unless the scenario was documented and the function is bugged).</p>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">It is undocumented what the users should do. Some avoid calling the
_deinit on errors, but others do not. Unless you document it, the
segfault from freeing uninit memory is not the user's fault, but a bug
in gnutls' documentation. But even if you document that the memory
remains uninit on failure, you have NOT actually uniformly implemented
that, and there is EXISTING code in the wild that DOES call the
_deinit function, and therefore WILL crash.</p>
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0.5rem 0;" align="initial">
<p style="color: inherit; line-height: 1.5; margin: 0 0 16px;">I would suggest either that</p>
<ul style="margin: 0 0 16px; padding: 0;">
<li style="margin-top: 0; line-height: 1.6em; margin-left: 25px; padding-left: 3px;">
<code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; margin-top: 0; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>gnutls_init</code> will not modify the output argument on error.</li>
</ul>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">Doable, but a LOT more churn (as I said, I already did the audit, and
more than 80% of the *_init functions would need changing).</p>
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0.5rem 0;" align="initial">
<p style="color: inherit; line-height: 1.5; margin: 0 0 16px;">or</p>
<ul style="margin: 0 0 16px; padding: 0;">
<li style="margin-top: 0; line-height: 1.6em; margin-left: 25px; padding-left: 3px;">
<code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; margin-top: 0; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>gnutls_init</code> will always set the output argument to some defined value on error. (in this case <code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>NULL</code>)</li>
</ul>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">That was my preference as well; see
<a href="https://gitlab.com/gnutls/gnutls/-/merge_requests/1652" data-reference-type="merge_request" data-original="https://gitlab.com/gnutls/gnutls/-/merge_requests/1652" data-link="false" data-link-reference="true" data-project="179611" data-merge-request="182790240" data-project-path="gnutls/gnutls" data-iid="1652" data-container="body" data-placement="top" title="gnutls_init: Always initialize *session" class="gfm gfm-merge_request" style="margin-top: 0;">!1652</a></p>
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0.5rem 0;" align="initial">
<p style="color: inherit; line-height: 1.5; margin: 0;">In either case, the documentation of <code style='font-size: 90%; color: #1f1f1f; word-wrap: break-word; background-color: #f0f0f0; border-radius: 4px; margin-top: 0; font-weight: inherit; font-family: "Menlo", "DejaVu Sans Mono", "Liberation Mono", "Consolas", "Ubuntu Mono", "Courier New", "andale mono", "lucida console", monospace; white-space: pre-wrap; overflow-wrap: break-word; word-break: keep-all; padding: 2px 4px;'>gnutls_init</code> should be updated to clearly state what happens with session when error occurs.</p>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">My preference as well, also in the merge commit (well, for
gnutls_init(); but if desired, I could expend the documentation
efforts to repeat that information for all *_init functions).</p>
<blockquote dir="auto" style="font-size: inherit; color: #525252; padding-top: 0.5rem; padding-bottom: 0.5rem; padding-left: 1.5rem; box-shadow: inset 4px 0 0 0 #dbdbdb; border-top-width: 0; border-bottom-width: 0; border-right-width: 0; margin: 0.5rem 0;" align="initial">
<p style="color: inherit; line-height: 1.5; margin: 0;">I would prefer to not touch the argument on error but both solutions are ok imo.</p>
</blockquote>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">I hope my merge commit can persuade you to ALWAYS touch the argument
on all exit paths, while at the same time documenting that the usesr
need not call the _deinit function on failure, and furthermore that
the user MAY (but not MUST) pre-zero the memory before calling _init,
such that even if the call _deinit on failure, it is safe in (almost)
all cases. The lone exception is gnutls_pkcs11_privkey_init(), where
the use of the wrong free() function causes certain failures to cause
a double-free if the user calls _deinit on certain failures.</p>
<p dir="auto" style="color: #303030; margin: 0 0 16px;" align="initial">Blindly claiming that it is the user's fault for segfaulting on deinit
after failure when gnutls documentation did not make a strong
guarantee on what the user should do is not a very nice attitude for
security-sensitive software to be portraying.</p>
<details style="margin-bottom: 0;"><summary style="margin-top: 0;">...</summary>
<p style="color: #303030; margin: 0 0 16px 1rem;">On Wed, Oct 19, 2022 at 03:12:18PM +0000, Zoltán Fridrich (<a href="https://gitlab.com/ZoltanFridrich" data-reference-type="user" data-user="8380993" data-container="body" data-placement="top" class="gfm gfm-project_member js-user-link" title="Zoltán Fridrich" style="background-color: #cbe2f9; border-radius: 4px; color: #0b5cad; margin-top: 0; padding: 0 2px;">@ZoltanFridrich</a>) wrote:</p>
<p style="color: #303030; margin: 0 0 16px 1rem;">--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org</p>
</details>
</div>
</div>
<div class="footer" style="margin-top: 10px;">
<p style="font-size: small; color: #666;">
—
<br>
Reply to this email directly or <a href="https://gitlab.com/gnutls/gnutls/-/issues/1414#note_1143168550">view it on GitLab</a>.
<br>
You're receiving this email because of your account on <a target="_blank" rel="noopener noreferrer" href="https://gitlab.com">gitlab.com</a>. <a href="https://gitlab.com/-/sent_notifications/ca9c236479c09ee14c22ea297b5a7c63/unsubscribe" target="_blank" rel="noopener noreferrer">Unsubscribe</a> from this thread · <a href="https://gitlab.com/-/profile/notifications" target="_blank" rel="noopener noreferrer" class="mng-notif-link">Manage all notifications</a> · <a href="https://gitlab.com/help" target="_blank" rel="noopener noreferrer" class="help-link">Help</a>
<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","action":{"@type":"ViewAction","name":"View Issue","url":"https://gitlab.com/gnutls/gnutls/-/issues/1414#note_1143168550"}}</script>
</p>
</div>
</body>
</html>