On Fri, 09 Dec 2011 15:10:47 +0100 Jakob Bohm <jb-open...@wisemo.com> wrote:
> Hi, nice code, I spot a few questionable details, but only Warn#5 > might cause missing bytes. > > if (!field) return newSV(0); > Warn#1: It is probably more efficient to return PL_sv_undef, avoiding > an allocation in a potential memory full situation Point taken. > Bug #2: must be allocated as [len+1] because of Bug#7 below. WRT to #2 and #7, the caveat I've seen about this has to do with handing a perl string into a C function that treats it like a C-string. Eg, length() and SvPV(data, 0) use strlen. However, if you set a length manually, = and .= will use that. Otherwise putting binary data into a perl string would be near pointless. Ie, it's not essential to null terminate here as long as you are aware of the potential for runaway string opts. However, after exhausting the other possibilities, I tried this. Kind of a hassle because I then had to add chop() here and there. But lo and behold, it worked. Considering each call of the sysread only got 16KB at most -- meaning it is called thousands of times for a 20 MB upload -- I'm baffled as to why this would cause such a small and irregular loss. The POST is not accumulated using perl either, or C string functions, just pointers. I don't like baffled, I like to think I know why ;) Good thing the perlish are everywhere. Thanks much -- I have a few more comments/questions about your comments if you're interested. > Warn#3: It is probably more efficient to do > SvGrow(buf, len + 1); > unsigned char *data = SvPV_nolen(buf); Good idea; I then have to reset the length to the actual bytes read, but this will save a copy. Thanks. > Warn#4: The calling perl code may need to distinguish between > SSL_ERROR_WANT_READ > and SSL_ERROR_WANT_WRITE, because the needed select() > call will be different I haven't actually seen SSL_ERROR_WANT_WRITE happen here; initially I was testing for them separately. In the SSL_read man page, it says: "As at any time a re-negotiation is possible, a call to SSL_read() can also cause write operations! The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_read (). The action depends on the underlying BIO . When using a non-blocking socket, *nothing is to be done*..." So, if I were to handle them differently, how would I do it? > Warn#5: Remember to ensure the perl code passes the exact same > parameters on retry! Yep. I actually made the "data" buffer global temporarily to make sure the address stays the same, no change. And the length remaining (based on bytes read and content-length) is used for the len arg; than will not change if the last call returned nothing. > > // return buffer contents to perl > > sv_setpvn(buf, data, bytes); > Bug#8: Note that if bytes==0 (a valid situation), then sv_setpvn() > Bug#will > act like sv_setpvn(buf, data, strlen(data)) > So in addition to Bug#7 above, bytes==0 could turn into > a variable number of random bytes getting put in buf. Good catch, I should change that to prevent the wasted allocation. However, it would not matter WRT to the collection of data, because this function returns "bytes" (via SvIV). Currently I'm treating 0 the same way I would with a normal socket -- as an indication that the client disconnected. Does not seem to be a problem thus far (ie, SSL_read never returns 0 during the transfer). I left the \0 out of the byte count so this does not get screwed up (but allocated enough in SvCUR_set). Anyway, seemingly the problem is solved! Phew. But if you think I'm off base about anything here, I'm listening. :) More thanks -- MK -- "Enthusiasm is not the enemy of the intellect." (said of Irving Howe) "The angel of history[...]is turned toward the past." (Walter Benjamin) ______________________________________________________________________ OpenSSL Project http://www.openssl.org User Support Mailing List openssl-users@openssl.org Automated List Manager majord...@openssl.org
