crypt_data: structure size in crypt.c vs. crypt.h

crypt_data: structure size in crypt.c vs. crypt.h

[Zvi Gilboa]

[-- Attachment #1.1: Type: text/plain, Size: 840 bytes --]

Greetings,

Looking at crypt.c and crypt.h, I was wondering whether having 
different-size /crypt_data/ structures was intentional, and if so, for 
what reason.  At the moment, the declarations are (and see also the 
attached code):

/<crypt.h>/
struct crypt_data {
     int initialized;
     char *__buf[256]*;
}; /* 260 bytes when sizeof(int)==4 */

/<crypt.c>/
char *__crypt_r(const char *, const char *, *struct crypt_data **);

char *crypt(const char *key, const char *salt)
{
     static char *buf[128]*;
     return __crypt_r(key, salt, *(struct crypt_data *)buf*);
     /* when sizeof(int)==4, this leaves __buf with 124 bytes. */
}


On that note: since the /initialized /member is (currently) never 
referenced by name, adding a comment about that to the code might help 
readers who are yet to be initiated:)

Best regards,
Zvi


[-- Attachment #1.2: Type: text/html, Size: 1359 bytes --]

[-- Attachment #2: crypt_data_struct_size.c --]
[-- Type: text/x-csrc, Size: 264 bytes --]

#include <stdio.h>
#include <crypt.h>

int main(void) {
	struct crypt_data cd;
	static char buf[128];
	
	printf("sizeof(int)\t\t\t%zu\n", sizeof(int));
	printf("sizeof(crypt_data)\t\t%zu\n", sizeof(cd));
	printf("sizeof(static char[128])\t%zu\n", sizeof(buf));
}

Re: crypt_data: structure size in crypt.c vs. crypt.h

[Rich Felker]

On Thu, Apr 11, 2013 at 06:42:26PM -0400, Zvi Gilboa wrote:
> Greetings,
> 
> Looking at crypt.c and crypt.h, I was wondering whether having
> different-size /crypt_data/ structures was intentional, and if so,
> for what reason.  At the moment, the declarations are (and see also
> the attached code):

Yes and no. struct crypt_data becomes part of the ABI of the program
that calls crypt_r, so it needs to be "large enough for any future
password hash". The number 256 was chosen as a nice round value. On
the other hand, the static buffer used by crypt() only needs to be
"large enough for any currently-supported password hash", which is
presently 128.

> /<crypt.h>/
> struct crypt_data {
>     int initialized;
>     char *__buf[256]*;
> }; /* 260 bytes when sizeof(int)==4 */
> 
> /<crypt.c>/
> char *__crypt_r(const char *, const char *, *struct crypt_data **);
> 
> char *crypt(const char *key, const char *salt)
> {
>     static char *buf[128]*;
>     return __crypt_r(key, salt, *(struct crypt_data *)buf*);
>     /* when sizeof(int)==4, this leaves __buf with 124 bytes. */

No, the size is the full 128. There really is no such member as
"initialized" as far as musl's __crypt_r is concerned; the whole
object is used simply as a char[] buffer. The only reason the
"initialized" member exists is to meet the API requirements of the
crypt_r function -- per the API, applications are supposed to either
set crypt_data.initialized=0, or zero-initialize the whole object,
before calling crypt_r. If a member named "initialized" did not exist,
such programs would fail to compile. However, musl itself has no use
for any initialized member, since it only uses crypt_data as an output
buffer, not a working buffer for crypto state.

> On that note: since the /initialized /member is (currently) never
> referenced by name, adding a comment about that to the code might
> help readers who are yet to be initiated:)

Where do you think would be best to add it? In __crypt_r? The
"natural" place would be the headers, but the current policy is to
avoid comments in the headers, especially ones which would be
implementation-specific rather than documenting the API.

Rich

Re: crypt_data: structure size in crypt.c vs. crypt.h

[Zvi Gilboa]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On 04/11/2013 06:59 PM, Rich Felker wrote:
> On Thu, Apr 11, 2013 at 06:42:26PM -0400, Zvi Gilboa wrote:
>> Greetings,
>>
>> Looking at crypt.c and crypt.h, I was wondering whether having
>> different-size /crypt_data/ structures was intentional, and if so,
>> for what reason.  At the moment, the declarations are (and see also
>> the attached code):
> Yes and no. struct crypt_data becomes part of the ABI of the program
> that calls crypt_r, so it needs to be "large enough for any future
> password hash". The number 256 was chosen as a nice round value. On
> the other hand, the static buffer used by crypt() only needs to be
> "large enough for any currently-supported password hash", which is
> presently 128.
>
>> /<crypt.h>/
>> struct crypt_data {
>>      int initialized;
>>      char *__buf[256]*;
>> }; /* 260 bytes when sizeof(int)==4 */
>>
>> /<crypt.c>/
>> char *__crypt_r(const char *, const char *, *struct crypt_data **);
>>
>> char *crypt(const char *key, const char *salt)
>> {
>>      static char *buf[128]*;
>>      return __crypt_r(key, salt, *(struct crypt_data *)buf*);
>>      /* when sizeof(int)==4, this leaves __buf with 124 bytes. */
> No, the size is the full 128. There really is no such member as
> "initialized" as far as musl's __crypt_r is concerned; the whole
> object is used simply as a char[] buffer. The only reason the
> "initialized" member exists is to meet the API requirements of the
> crypt_r function -- per the API, applications are supposed to either
> set crypt_data.initialized=0, or zero-initialize the whole object,
> before calling crypt_r. If a member named "initialized" did not exist,
> such programs would fail to compile. However, musl itself has no use
> for any initialized member, since it only uses crypt_data as an output
> buffer, not a working buffer for crypto state.
>
>> On that note: since the /initialized /member is (currently) never
>> referenced by name, adding a comment about that to the code might
>> help readers who are yet to be initiated:)
> Where do you think would be best to add it? In __crypt_r? The
> "natural" place would be the headers, but the current policy is to
> avoid comments in the headers, especially ones which would be
> implementation-specific rather than documenting the API.

Thanks for the explanation!  Since crypt.c is likely to be visited by 
anyone going through the crypt-related code, I'd suggest placing the 
comment there (proposed comment attached).

>
> Rich


[-- Attachment #2: crypt_data_comment.patch --]
[-- Type: text/x-patch, Size: 917 bytes --]

From b9d9d4c46486b4f0e77cf7b4455de8ba87c9f5ef Mon Sep 17 00:00:00 2001
From: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
Date: Thu, 11 Apr 2013 19:52:05 -0400
Subject: [PATCH] Committer: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>

On branch crypt_data_comment
---
 src/crypt/crypt.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/crypt/crypt.c b/src/crypt/crypt.c
index f1e310f..fdc4427 100644
--- a/src/crypt/crypt.c
+++ b/src/crypt/crypt.c
@@ -1,6 +1,12 @@
 #include <unistd.h>
 #include <crypt.h>
 
+/***
+  struct crypt_data       --> the entire structure is used as a char[] buffer
+        int initialized   --> member exists to satisfy API requirements, currently not used
+        char __buf[256];  --> large enough for any future password hash
+ ***/
+
 char *__crypt_r(const char *, const char *, struct crypt_data *);
 
 char *crypt(const char *key, const char *salt)
-- 
1.7.9.5

Re: crypt_data: structure size in crypt.c vs. crypt.h

[Rich Felker]

On Thu, Apr 11, 2013 at 08:03:15PM -0400, Zvi Gilboa wrote:
> Thanks for the explanation!  Since crypt.c is likely to be visited
> by anyone going through the crypt-related code, I'd suggest placing
> the comment there (proposed comment attached).
> 
> [...]
> >From b9d9d4c46486b4f0e77cf7b4455de8ba87c9f5ef Mon Sep 17 00:00:00 2001
> From: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
> Date: Thu, 11 Apr 2013 19:52:05 -0400
> Subject: [PATCH] Committer: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
> 
> On branch crypt_data_comment
> ---
>  src/crypt/crypt.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/crypt/crypt.c b/src/crypt/crypt.c
> index f1e310f..fdc4427 100644
> --- a/src/crypt/crypt.c
> +++ b/src/crypt/crypt.c
> @@ -1,6 +1,12 @@
>  #include <unistd.h>
>  #include <crypt.h>
>  
> +/***
> +  struct crypt_data       --> the entire structure is used as a char[] buffer
> +        int initialized   --> member exists to satisfy API requirements, currently not used
> +        char __buf[256];  --> large enough for any future password hash
> + ***/
> +

I understand your motivation, but I don't want to introduce comments
like this that duplicate definitions from elsewhere. I think the
comments should be in crypt.c and crypt_r.c, and should be something
along the line of:

(crypt.c) This buffer is sufficiently large for all
currently-supported hash types. It needs to be updated if longer
hashes are added. The cast to struct crypt_data * is purely to meet
the public API requirements of the crypt_r function; the
implementation of crypt_r uses the object purely as a char buffer.

(crypt_r.c) Per the crypt_r API, the caller has provided a pointer to
struct crypt_data; however, this implementation does not use the
structure to store any internal state, and treats it purely as a char
buffer for storing the result.

Would these comments have been sufficient for you to understand what
was going on when you first read the source? If so, I'll go ahead and
add them; if not, what would you like to see improved?

Rich

Re: crypt_data: structure size in crypt.c vs. crypt.h

[Zvi Gilboa]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

On 04/11/2013 09:42 PM, Rich Felker wrote:
> On Thu, Apr 11, 2013 at 08:03:15PM -0400, Zvi Gilboa wrote:
>> Thanks for the explanation!  Since crypt.c is likely to be visited
>> by anyone going through the crypt-related code, I'd suggest placing
>> the comment there (proposed comment attached).
>>
>> [...]
>> >From b9d9d4c46486b4f0e77cf7b4455de8ba87c9f5ef Mon Sep 17 00:00:00 2001
>> From: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
>> Date: Thu, 11 Apr 2013 19:52:05 -0400
>> Subject: [PATCH] Committer: Zvi Gilboa <zgilboa@zgilboa-S14Y-linux.(none)>
>>
>> On branch crypt_data_comment
>> ---
>>   src/crypt/crypt.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/src/crypt/crypt.c b/src/crypt/crypt.c
>> index f1e310f..fdc4427 100644
>> --- a/src/crypt/crypt.c
>> +++ b/src/crypt/crypt.c
>> @@ -1,6 +1,12 @@
>>   #include <unistd.h>
>>   #include <crypt.h>
>>   
>> +/***
>> +  struct crypt_data       --> the entire structure is used as a char[] buffer
>> +        int initialized   --> member exists to satisfy API requirements, currently not used
>> +        char __buf[256];  --> large enough for any future password hash
>> + ***/
>> +
> I understand your motivation, but I don't want to introduce comments
> like this that duplicate definitions from elsewhere. I think the
> comments should be in crypt.c and crypt_r.c, and should be something
> along the line of:
>
> (crypt.c) This buffer is sufficiently large for all
> currently-supported hash types. It needs to be updated if longer
> hashes are added. The cast to struct crypt_data * is purely to meet
> the public API requirements of the crypt_r function; the
> implementation of crypt_r uses the object purely as a char buffer.
>
> (crypt_r.c) Per the crypt_r API, the caller has provided a pointer to
> struct crypt_data; however, this implementation does not use the
> structure to store any internal state, and treats it purely as a char
> buffer for storing the result.
>
> Would these comments have been sufficient for you to understand what
> was going on when you first read the source? If so, I'll go ahead and
> add them; if not, what would you like to see improved?

Thank you, Rich, the above would have made things perfectly clear -- and 
hopefully would benefit future readers/porters as well. And while we are 
at it...

-- has there ever been an attempt (or an intention) to create scripts 
that would turn the source tree into a "book" (a la /literate 
programming/)?  The lightness of /nuweb/ (http://nuweb.sourceforge.net/) 
should make it a perfect match for musl, and it should not be too 
complicated to create some "reverse literate programming" scripts 
(reverse, since in "straight" literate programming the C course files 
are the output, whereas here they will be used as the input).

-- any general interest in a command-line utility that provides the 
Wiki-based build status of a specific package?  I have just create one, 
and will be happy to "generalize" it.

>
> Rich


[-- Attachment #2: Type: text/html, Size: 3866 bytes --]