<div dir="ltr"><span class="">Checks</span> from static function are removed. Kindly review it.<div class="gmail_extra"><br clear="all"><div>With regards<br>Pallavi Kumari Jha<br>Computer Science Engineering<br></div>
<br><br><div class="gmail_quote">On 8 December 2013 03:25, Lukas Slebodnik <span dir="ltr">&lt;<a href="mailto:lslebodn@redhat.com" target="_blank">lslebodn@redhat.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div><div>On (08/12/13 02:05), Pallavi Jha wrote:<br>
&gt;Hi,<br>
&gt;<br>
&gt;Requested changes have been done. Kindly review the patch.<br>
&gt;<br>
&gt;With regards<br>
&gt;Pallavi Kumari Jha<br>
&gt;Computer Science Engineering<br>
&gt;<br>
&gt;<br>
&gt;On 22 November 2013 20:34, Jakub Hrozek &lt;<a href="mailto:jhrozek@redhat.com" target="_blank">jhrozek@redhat.com</a>&gt; wrote:<br>
&gt;<br>
&gt;&gt; On Fri, Nov 22, 2013 at 09:41:16AM -0500, Simo Sorce wrote:<br>
&gt;&gt; &gt; On Fri, 2013-11-22 at 18:42 +0530, Pallavi Jha wrote:<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt; Hello,<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt; Patch with required changes is attached with the mail. Please review<br>
&gt;&gt; &gt; &gt; it.<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; Thank you, comments inline.<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; &gt; From bfcbe7d16e325dffb807da096a27955c579174e0 Mon Sep 17 00:00:00 2001<br>
&gt;&gt; &gt; &gt; From: Pallavi Jha &lt;<a href="mailto:pallavikumarijha@gmail.com" target="_blank">pallavikumarijha@gmail.com</a>&gt;<br>
&gt;&gt; &gt; &gt; Date: Sat, 16 Nov 2013 16:23:01 +0530<br>
&gt;&gt; &gt; &gt; Subject: [PATCH] added null checks to authtok module<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt; ---<br>
&gt;&gt; &gt; &gt;  src/util/authtok.c | 30 ++++++++++++++++++++++++++++--<br>
&gt;&gt; &gt; &gt;  1 file changed, 28 insertions(+), 2 deletions(-)<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt; diff --git a/src/util/authtok.c b/src/util/authtok.c<br>
&gt;&gt; &gt; &gt; index<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; 83e6a1c942a0abc136422bf062be47cbc687bb72..2c67c038bfbeebade462b15eef72e7827fb9f7e4<br>
&gt;&gt; 100644<br>
&gt;&gt; &gt; &gt; --- a/src/util/authtok.c<br>
&gt;&gt; &gt; &gt; +++ b/src/util/authtok.c<br>
&gt;&gt; &gt; &gt; @@ -27,11 +27,17 @@ struct sss_auth_token {<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token<br>
&gt;&gt; &gt; &gt; *tok)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return -1;<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; -1 is not a valid sss_authtok_type, maybe we should just return<br>
&gt;&gt; &gt; SSS_AUTHTOK_TYPE_EMPTY, or you need to add -1 to the enumeration with an<br>
&gt;&gt; &gt; appropriate name SSS_AUTHTOK_TYPE_FAULTY ?<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; I wonder also if we should just let also some public functions fail,<br>
&gt;&gt; &gt; this should simply never happen ...<br>
&gt;&gt;<br>
&gt;&gt; Thanks Simo, we were discussing the same with Lukas right now and were<br>
&gt;&gt; not sure either :-)<br>
&gt;&gt;<br>
&gt;&gt; I&#39;m in favor or not checking this particular function.<br>
&gt;&gt; SSS_AUTHTOK_TYPE_EMPTY is a valid type, while NULL authtok is simply<br>
&gt;&gt; invalid. I would rather add SSS_AUTHTOK_TYPE_INVALID or similar, but<br>
&gt;&gt; adding an enum type just for a check (and then having to handle it in<br>
&gt;&gt; all the switch-case statements in all consumers) seems too much.<br>
&gt;&gt;<br>
&gt;&gt; So I&#39;m in favor of removing this check and only checking the other<br>
&gt;&gt; functions.<br>
&gt;&gt;<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      return tok-&gt;type;<br>
&gt;&gt; &gt; &gt;  }<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  size_t sss_authtok_get_size(struct sss_auth_token *tok)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return 0;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      switch (tok-&gt;type) {<br>
&gt;&gt; &gt; &gt;      case SSS_AUTHTOK_TYPE_PASSWORD:<br>
&gt;&gt; &gt; &gt;      case SSS_AUTHTOK_TYPE_CCFILE:<br>
&gt;&gt; &gt; &gt; @@ -45,12 +51,18 @@ size_t sss_authtok_get_size(struct sss_auth_token<br>
&gt;&gt; &gt; &gt; *tok)<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return NULL;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      return tok-&gt;data;<br>
&gt;&gt; &gt; &gt;  }<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  errno_t sss_authtok_get_password(struct sss_auth_token *tok,<br>
&gt;&gt; &gt; &gt;                                   const char **pwd, size_t *len)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return EINVAL;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; Perhaps EFAULT (as in bad address) is a better error message here ?<br>
&gt;&gt; &gt; Not 100% sure.<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; &gt;      switch (tok-&gt;type) {<br>
&gt;&gt; &gt; &gt;      case SSS_AUTHTOK_TYPE_EMPTY:<br>
&gt;&gt; &gt; &gt;          return ENOENT;<br>
&gt;&gt; &gt; &gt; @@ -70,6 +82,9 @@ errno_t sss_authtok_get_password(struct<br>
&gt;&gt; &gt; &gt; sss_auth_token *tok,<br>
&gt;&gt; &gt; &gt;  errno_t sss_authtok_get_ccfile(struct sss_auth_token *tok,<br>
&gt;&gt; &gt; &gt;                                 const char **ccfile, size_t *len)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return EINVAL;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      switch (tok-&gt;type) {<br>
&gt;&gt; &gt; &gt;      case SSS_AUTHTOK_TYPE_EMPTY:<br>
&gt;&gt; &gt; &gt;          return ENOENT;<br>
&gt;&gt; &gt; &gt; @@ -91,11 +106,16 @@ static errno_t sss_authtok_set_string(struct<br>
&gt;&gt; &gt; &gt; sss_auth_token *tok,<br>
&gt;&gt; &gt; &gt;                                        const char *context_name,<br>
&gt;&gt; &gt; &gt;                                        const char *str, size_t len)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok || !str) {<br>
&gt;&gt; &gt; &gt; +        return EINVAL;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt; +<br>
&gt;&gt;<br>
&gt;&gt; Please remove the check here, it is a static function and their API<br>
&gt;&gt; doesn&#39;t need to be checked.<br>
&gt;&gt;<br>
&gt;&gt; &gt; &gt;      size_t size;<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;      if (len == 0) {<br>
&gt;&gt; &gt; &gt;          len = strlen(str);<br>
&gt;&gt; &gt; &gt; -    } else {<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt; +    else {<br>
&gt;&gt;<br>
&gt;&gt; Also please revert this change, we use the following coding style in<br>
&gt;&gt; SSSD:<br>
&gt;&gt;<br>
&gt;&gt; if () {<br>
&gt;&gt; } else {<br>
&gt;&gt; }<br>
&gt;&gt;<br>
&gt;&gt; &gt; &gt;          while (len &gt; 0 &amp;&amp; str[len - 1] == &#39;\0&#39;) len--;<br>
&gt;&gt; &gt; &gt;      }<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt; @@ -121,6 +141,9 @@ static errno_t sss_authtok_set_string(struct<br>
&gt;&gt; &gt; &gt; sss_auth_token *tok,<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  void sss_authtok_set_empty(struct sss_auth_token *tok)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!tok) {<br>
&gt;&gt; &gt; &gt; +        return;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      switch (tok-&gt;type) {<br>
&gt;&gt; &gt; &gt;      case SSS_AUTHTOK_TYPE_EMPTY:<br>
&gt;&gt; &gt; &gt;          return;<br>
&gt;&gt; &gt; &gt; @@ -174,6 +197,9 @@ errno_t sss_authtok_set(struct sss_auth_token<br>
&gt;&gt; &gt; &gt; *tok,<br>
&gt;&gt; &gt; &gt;  errno_t sss_authtok_copy(struct sss_auth_token *src,<br>
&gt;&gt; &gt; &gt;                           struct sss_auth_token *dst)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; +    if (!src || !dst) {<br>
&gt;&gt; &gt; &gt; +        return EINVAL;<br>
&gt;&gt; &gt; &gt; +    }<br>
&gt;&gt; &gt; &gt;      sss_authtok_set_empty(dst);<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;      if (src-&gt;type == SSS_AUTHTOK_TYPE_EMPTY) {<br>
&gt;&gt; &gt; &gt; @@ -205,7 +231,7 @@ struct sss_auth_token *sss_authtok_new(TALLOC_CTX<br>
&gt;&gt; &gt; &gt; *mem_ctx)<br>
&gt;&gt; &gt; &gt;<br>
&gt;&gt; &gt; &gt;  void sss_authtok_wipe_password(struct sss_auth_token *tok)<br>
&gt;&gt; &gt; &gt;  {<br>
&gt;&gt; &gt; &gt; -    if (tok-&gt;type != SSS_AUTHTOK_TYPE_PASSWORD) {<br>
&gt;&gt; &gt; &gt; +    if (!tok || tok-&gt;type != SSS_AUTHTOK_TYPE_PASSWORD) {<br>
&gt;&gt; &gt; &gt;          return;<br>
&gt;&gt; &gt; &gt;      }<br>
&gt;&gt; &gt;<br>
&gt;&gt; &gt; Aside from the q. of whether EFAULT may be more appropriate in this case<br>
&gt;&gt; &gt; the rest looks sane.<br>
&gt;&gt;<br>
&gt;&gt; Looks good to me too.<br>
&gt;&gt; _______________________________________________<br>
&gt;&gt; sssd-devel mailing list<br>
&gt;&gt; <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
&gt;&gt; <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
&gt;&gt;<br>
<br>
</div></div>&gt;From 813e6cf4b275b44af6b4a2c56cca63a8366f4224 Mon Sep 17 00:00:00 2001<br>
<div>&gt;From: Pallavi Jha &lt;<a href="mailto:pallavikumarijha@gmail.com" target="_blank">pallavikumarijha@gmail.com</a>&gt;<br>
&gt;Date: Sat, 16 Nov 2013 16:23:01 +0530<br>
&gt;Subject: [PATCH] added null checks to authtok module<br>
&gt;<br>
&gt;---<br>
</div>&gt; src/util/authtok.c | 23 ++++++++++++++++++++++-<br>
&gt; 1 file changed, 22 insertions(+), 1 deletion(-)<br>
&gt;<br>
&gt;diff --git a/src/util/authtok.c b/src/util/authtok.c<br>
&gt;index 83e6a1c942a0abc136422bf062be47cbc687bb72..389e50504d943263c72e47a4d11dc818c2816565 100644<br>
&gt;--- a/src/util/authtok.c<br>
&gt;+++ b/src/util/authtok.c<br>
&gt;@@ -32,6 +32,9 @@ enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token *tok)<br>
<div>&gt;<br>
&gt; size_t sss_authtok_get_size(struct sss_auth_token *tok)<br>
&gt; {<br>
&gt;+    if (!tok) {<br>
&gt;+        return 0;<br>
&gt;+    }<br>
&gt;     switch (tok-&gt;type) {<br>
&gt;     case SSS_AUTHTOK_TYPE_PASSWORD:<br>
&gt;     case SSS_AUTHTOK_TYPE_CCFILE:<br>
</div>&gt;@@ -45,12 +48,18 @@ size_t sss_authtok_get_size(struct sss_auth_token *tok)<br>
<div>&gt;<br>
&gt; uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)<br>
&gt; {<br>
&gt;+    if (!tok) {<br>
&gt;+        return NULL;<br>
&gt;+    }<br>
&gt;     return tok-&gt;data;<br>
&gt; }<br>
&gt;<br>
&gt; errno_t sss_authtok_get_password(struct sss_auth_token *tok,<br>
&gt;                                  const char **pwd, size_t *len)<br>
&gt; {<br>
&gt;+    if (!tok) {<br>
</div>&gt;+        return EFAULT;<br>
<div>&gt;+    }<br>
&gt;     switch (tok-&gt;type) {<br>
&gt;     case SSS_AUTHTOK_TYPE_EMPTY:<br>
&gt;         return ENOENT;<br>
</div>&gt;@@ -70,6 +79,9 @@ errno_t sss_authtok_get_password(struct sss_auth_token *tok,<br>
<div>&gt; errno_t sss_authtok_get_ccfile(struct sss_auth_token *tok,<br>
&gt;                                const char **ccfile, size_t *len)<br>
&gt; {<br>
&gt;+    if (!tok) {<br>
&gt;+        return EINVAL;<br>
&gt;+    }<br>
&gt;     switch (tok-&gt;type) {<br>
&gt;     case SSS_AUTHTOK_TYPE_EMPTY:<br>
&gt;         return ENOENT;<br>
</div>&gt;@@ -91,6 +103,9 @@ static errno_t sss_authtok_set_string(struct sss_auth_token *tok,<br>
<div>&gt;                                       const char *context_name,<br>
&gt;                                       const char *str, size_t len)<br>
&gt; {<br>
&gt;+    if (!tok || !str) {<br>
&gt;+        return EINVAL;<br>
&gt;+    }<br>
</div>     ^^^^^^^^^^^^^^^^^^^^<br>
You probably missed Jakub&#39;s comment in the previous mail.<br>
<div><br>
&gt;&gt;Please remove the check here, it is a static function and their API<br>
&gt;&gt;doesn&#39;t need to be checked.<br>
<br>
</div><span><font color="#888888">LS<br>
</font></span><div><div>_______________________________________________<br>
sssd-devel mailing list<br>
<a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
<a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
</div></div></blockquote></div><br></div></div>