348366 - ber_printf support for O format

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, enhancement)

Description

[Ulf Weltman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

We support the "O" format (berval struct as opposed to passing value and length as separate arguments) for ber_scanf but not ber_printf.  If I understand correctly for ber_printf the difference between the existing "o" format and missing "O" is:
ber_printf(ber, "o", (char*)bv.bv_val, (int)bv.bv_len)
vs
ber_printf(ber, "O", (struct berval *)bv)

It seems like just a convenience thing, so maybe it's just that no one needed it enough to add it in the past - or is there something more behind it?

Reproducible: Always

Comment 1

[Mark Smith [:mcs]]

The ber_ APIs are (generally) private to libldap and not widely used by application writers.  I suppose we just never saw the need for 'O'.  Do you need it?  Do you have a patch?  According to the OpenLDAP manual page they do support 'O'.

Comment 2

[Ulf Weltman]

Attachment: Patch proposal

I was thinking something like what's in this attachment.  I haven't tested it well yet.

It's requested by someone working on cifs/samba at HP.  I suspect they can use the "o" format as a workaround.  Not sure how "O" came up initially, maybe there's code being ported from using OL to Mozilla SDK.

Comment 3

[Rich Megginson]

Hmm - OL has some other nifty flags, like !, which allows you to pass in a function and context arg so that you can customize what action you want to take with the printf/scanf.  I'm not really sure what you would want to use that for, given the limited number of ber types that cover everything in the LDAP protocol.  It also allows you to scan into a string/bv without allocating memory - it just gives you a pointer into the raw ber, which is very useful in proxy applications.

What would be really useful is a malloc avoiding option for strings/bvals - you could pass in 3 items: a char*, a fixed size char[size], and a size.  If ber_scan detects that the value will overflow the fixed size buffer, it mallocs a new buffer and assigns the first char* to the new buffer.  If it has enough room already, it just assigns char* to the fixed size buffer.  The caller would have to have some logic like this:
if (bufptr != fixedbuf) {
  ldap_memfree(bufptr);
}
But that might save a considerable number of mallocs in the most common cases.

I'd like to be able to do the same thing with PR_smprintf - print into a fixed size buffer or malloc on overflow.

Comment 4

[Ulf Weltman]

This is the code being ported from OL SDK to Mozilla SDK, find the function ads_do_paged_search: 
http://viewcvs.samba.org/cgi-bin/viewcvs.cgi/branches/SAMBA_3_1_RELEASE/source/libads/ldap.c?rev=2563&view=markup

It's trying to send back the void** cookie from the previous simple-paged-results ADS search, "O" format does seem handy in this case.  As a local workaround I think she could use the "o" format after pointing a berval pointer to the cookie and accessing its members that way.

As for adding "O" support, do you guys spot any issues with the patch?  Any ideas for testing it besides watching it fix the paged search in the Samba code? 

Comment 5

[Rich Megginson]

The patch looks good.  As for testing it, I suppose you could write a simple test program that did a ber_init/ber_printf/ber_flatten/ber_scanf.  We'll try to get this patch in soon - we have a lot of other code to get in first.

Comment 6

[Mark Smith [:mcs]]

For maximum compatibility we should emulate the OpenLDAP behavior exactly.  They basically do this:

if( bv == NULL || bv->bv_len == 0 ) {
    rc = ber_put_ostring( ber, "", (ber_len_t) 0, tag );
else
    rc = ber_put_ostring( ber, bval->bv_val, bval->bv_len, tag );

Comment 7

[Ulf Weltman]

Attachment: New proposal, more OL compatible

But that function is only called if bv wasn't NULL, so if bv was NULL we shouldn't put an empty berval in the ber, right?  I'm not sure if they did that on purpose, it looks like it was intended that if either bv was NULL, or bv->bv_len was 0, it would put an empty berval in the ber.  But in practice it looks like the bv==NULL case just skips by now.

For even more compatibility maybe we could add that ber_put_berval function which "O" format uses but then I ought to update the "V" and "W" formats to use that function too, and I was hoping to not "fix what isn't broken".

Comment 8

[Mark Smith [:mcs]]

Comment on attachment 233712 [details] [diff] [review]
New proposal, more OL compatible

I think this patch is fine.  Let's not fix V and W at this point (I am sure there are many other incompatibilities we don't even know about).

Comment 9

[Rich Megginson]

Fix committed to the trunk.

Checking in encode.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/encode.c,v  <--  encode.c
new revision: 5.5; previous revision: 5.4
done

This bug can be resolved as FIXED.