|
That would be great and much appreciated. Just by way of note, however, version 2.3 of the library does support UFT-8 filenames for the attachments. It would be good for you to check over the implementation and make sure it is compliant, but it is already there.
Thanks,
David
|
|
|
|
|
In
int CSmtp::ConnectRemoteServer(...):
if(connect(hSocket,(LPSOCKADDR)&sockAddr,sizeof(sockAddr)) == SOCKET_ERROR)
{
...
}
else
{
return true; <======================= (??) ==============================
}
...
if(securityType!=DO_NOT_SET) SetSecurityType(securityType);
if(GetSecurityType() == USE_TLS || GetSecurityType() == USE_SSL)
{
InitOpenSSL();
if(GetSecurityType() == USE_SSL)
{
OpenSSLConnect();
}
}
modified 25-Jun-13 3:42am.
|
|
|
|
|
This part of the code probably doesn't work like you would expect based on typical syntax. The connect call is actually supposed to return a SOCKET_ERROR with the error code being WSAWOULDBLOCK so the else branch is avoided unless there is another error. See the explanation of WSAWOULDBLOCK here for more information:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx[^]
|
|
|
|
|
|
|
Tadkie,
Thanks for the suggestion. If you want to implement this and post back your proposed changes to the code to make it work I'd be happy to look at the result and add it to the main branch of the code.
Thanks,
David
|
|
|
|
|
Great article. This has helped me a lot. I hope I haven't sounded overly critical before because that certainly wasn't my intent.
Anyway here's a couple thoughts from looking at 2.2:
1. In CSmtp::Send (lines 535-553) you're missing an fclose(hFile) when opening attachments for the size check.
2. It'd be nice to have a ClearMessage function or some similar name that clears all of the "message specific" data. In other words, anything that refers to the actual email (recipients, attachments, body, etc.) and not anything referring to the server connection. This way it will be easy to send multiple emails during the same connection. This, of course, can done manually in the calling code, but a member function that handles this will make sure the caller doesn't miss anything when resetting for the next message.
An even better solution (although this is definitely more radical) is to separate out the connection info and the message info into separate classes. Then you'd have something like:
CSmtp connection;
CSmtpMessage m1, m2, m3;
// Configure messages here... e.g. m1.AddRecipient, etc.
connection.Send(m1);
connection.Send(m2);
connection.Send(m3);
3. What did you think of my idea to move the calls to DisconnectRemoteServer to only in the destructor for better exception safety?
|
|
|
|
|
GKarRacer,
Thanks for your input to the project. Your input has only been taken as intended to help the project get better for everyone's benefit. No offense taken whatsoever. Item 1 has been addressed and a patch of code to fix it and improve efficiency of adding attachments has been posted here:
http://www.codeproject.com/Messages/4562782/Re-retreiving-file-size-for-attachments.aspx
I like your idea on item 2. I've put that into release 2.3. As for the point about separating the message and the connection into two classes - actually I have done that in how I use the code. I came to this project from my own home-baked SMTP library to get the ability to use SSL. I just patched my old library into this one, which included a separate class for the message object. Maybe I should add that to the library.
I guess I thought it was a bit friendlier on the server to disconnect right away when a problem is encountered. Currently it's only called two other places: ConnectRemoteServer() and Send(), in both cases only when a known SMTP error is encountered. I don't think that is unsafe. I certainly value your further comments on it, though. Please post back your thoughts.
|
|
|
|
|
The reason its unsafe is that DisconnectRemoteServer can throw an exception that potentially won't be caught. This is especially a big no-no when inside a destructor. This causes the program to immediately terminate. There is also the potential of a double exception happening when Send (and maybe ConnectRemoteServer as well) throws an exception. Since DisconnectRemoteServer is called inside the catch handler for Send/ConnectRemoteServer, it's possible for another exception to get thrown. Unfortunately I'm a little unclear on the standard of what's supposed to happen in this case. I can tell you what Visual Studio 2010's implentation does: program terminates.
This exact scenario was happening with the original attachment code problem. I tried wrapping the DisconnectRemoteServer call (inside Send) with its own catch handler. The nested handler was able to catch the exception, but when Send's handler tried to rethrow the original exception, the program aborted. I tried many different variations, but every time I tried to throw from inside Send's catch handler, it aborted.
Now since the attachment code has been fixed, this scenario is less likely, but the potential still exists. The only solution I can think of at the moment that still lets the original exception propagate back up to the caller is to NOT call DisconnectRemoteServer from the Send/ConnectRemoteServer function and only call it from the destructor. But, absolutely ensuring that it is wrapped with its own try/catch block that does NOT propagate the exception.
Try it for yourself. Put in a deliberate throw ECSmtp from somewhere inside DisconnectRemoteServer and then make sure to throw while inside Send. See what happens.
|
|
|
|
|
Sorry to take so long thinking about this and looking it over multiple times. I wonder if we could just put a
try{
...
}catch(...){
}
handler surrounding all the code inside DisonnectRemoteServer and leave it as-is. I know that a throw; statement inside a catch block rethrows the existing exception. What I don't know for sure is if you have another exception thrown in the middle of that catch block, which exception does it re-throw? The original one or the one that ended up getting thrown inside the block? I would think as long as the second one was thrown and caught in a different function it would throw the original one. From what you described, I got the impression that you had tested putting another nested try{}catch{} block around the call the DisconnectRemoteServer, but had not tried putting it inside DisconnectRemoteServer. Is that correct? Do you have a way to test this alternative?
Thanks,
David
|
|
|
|
|
Sorry, just saw this. I haven't looked at it in a while, but if I remember correctly I believe I had tried it with nested catch blocks and it was still crashing as soon as I tried to rethrow the exception. I had even tried creating a new exception and throwing that instead, but alas it didn't work. Although I don't think I tried a generic catch handler. Honestly I'm not sure if the crash isn't just a bug in Visual Studio. I'm not sure exactly what the standard says about nested exceptions.
I'll have to pull up my test program and give it a whirl again. Hopefully I'll get a chance soon.
Edit:
OK, reread my original posts. I forgot I was talking about exceptions within the destructor. That part's very clear in the standard. That can't be allowed.
|
|
|
|
|
Hi David,
Can I used this code in commercial applications,
Thanks.
|
|
|
|
|
Yes, you may. It is subject to the CPOL:
http://www.codeproject.com/info/cpol10.aspx
It also carries with it certain obligations from OpenSSL, which you could find on their website, and obligations for including the MD5.cpp and MD5.h files, which you can read in their headers.
|
|
|
|
|
Hi,
i know we got this Topic already here but this is 3 years ago and the solution doesn't work at me.
So my question:
I want to compile the Project files in Dev-C++.
What i did:
-Download your package.
-Put the necessary files (*.h and *.cpp) into a new Project file.
-Add "openssl-0.9.8l\inc32" to Additional Include Directories with clicking on: Project -> Project Options -> Directories -> Include Directories
-Deleted the following lines in CSmtp.cpp:
#ifndef LINUX
#endif
Downloaded the openssl package for Dev-C++ from here: http://sourceforge.net/projects/devpaks/files/openssl/[^] and installed it with the Package Manager.
Then I clicked: Project -> Project Options -> Parameters -> then in the Linker Box:
-Ws2_32.lib
openssl-0.9.8l/out32/libeay32.lib
openssl-0.9.8l/out32/ssleay32.lib
But now i get 500 Linker Errors:
https://dl.dropboxusercontent.com/u/22986507/Linker%20Errors.txt[^]
The openssl Folder is in the Project Folder. So the structure is like that:
https://dl.dropboxusercontent.com/u/22986507/Data%20Structure.pdf[^]
Thanks for your help and i hope you understand me
|
|
|
|
|
I'm not familiar with Dev-C++ so I can't be of too much help. What I can say, however, is that when I first got started with this project I was using a different version of MSVC than what the project was using so I had to recompile OpenSSL with my compiler to get it to link properly. Have you compiled OpenSSL on your own Dev-C++ compiler or are you using a precompiled version that could have been compiled with a different version of Dev-C++?
David
|
|
|
|
|
Hi, thanks for your help.
I am using your precompiled OpenSSL. I only copied it into my project folder.
How i can compile it properly? Only compile every *.h for it own?
Wolkenpaul
|
|
|
|
|
I'm guessing you can find a guide on how to compile it using Dev-C++ on the OpenSSL website. I think that's where I found a guide for compiling it with MSVC.
|
|
|
|
|
|
If you attempt to send an attachment that is greater than the default limit of 5MB (say 6MB, for example), the program waits indefinitely until the server times out and closes the connection and then proceeds to crash.
Here's the sequence:
1. Start sending attachment.
2. Notice that the file is too large, throw exception MSG_TOO_BIG
3. Catch exception and call DisconnectRemoteServer.
4. Send QUIT command.
5. Waits for response.
6. Server times out waiting for message data.
7. My server responded with: 421 <server name=""> SMTP incoming data timeout - closing connection.
8. Server closes connection abruptly.
9. Select returns with response received in step 7 (421...)
10. Checks response.
11. 421 is not a valid response for QUIT.
12. Throws exception
13. Program aborts because it is a new exception is thrown that is unhandled (at least in the example program).
The underlying problem here (at least in this case) is that the QUIT command is not valid because it is currently inside the message data portion. The message data must be ended first before the server can start accepting commands again. I suggest sending the end of message line before throwing for the failed attachment. Also perhaps it's better to check the file's validity before even putting out the MIME header data for it.
The secondary problem is the unhandled exception. I suggest wrapping the call to DisconnectRemoteServer (from inside the Send catch handler) with its own try/catch block. These exceptions are then trapped and ignored. Unfortunately I'm not sure what to do about the original exception. I've tried different methods, but in each one I've tried the program aborts when rethrowing the original exception.
Hopefully this makes sense. There may be other failure points inside the message data section as well that have this issue, but this is what I found so far.
|
|
|
|
|
GKarRacer,
Which version of the project are you using? This is one of the primary issues resolved in version 2.2 of the code - at least I think. I just want to make sure if you are seeing an issue with it that you are on the latest version of the project.
Thanks,
David
|
|
|
|
|
I have version 2.1. I had downloaded it fairly recently and didn't realize there was an update out already. I will check it out. Thanks.
|
|
|
|
|
I took a quick look at 2.2. I haven't had a chance to run it yet, but the attachment handling does look better. Thanks.
There is still an issue, however, with exceptions occurring within DisconnectRemoteServer. If an exception occurs while calling this, the program most certainly will crash due to nested exceptions or worse by throwing from within the destructor. I think for safety all calls to DisconnectRemoteServer need to have its own try/catch block wrapper and these exceptions should not be forwarded onto the caller. The destructor for CSmtp especially needs this.
To get around the double exception problem in the Send function I mentioned earlier, here's a suggestion: Don't call DisconnectRemoteServer from within Send's catch handler. Let the destructor handle it during the normal stack unwind from the original exception. Make sure, of course, to wrap DisconnectRemoteServer in the destructor within its own try/catch block. If you do this Send doesn't even need its own try/catch block.
Come to think of it instead of always wrapping DisconnectRemoteServer, you could simply have the DisconnectRemoteServer function have its own internal try/catch block that eats all exceptions.
I think ConnectRemoteServer also may have the same issue as Send.
What do you think?
|
|
|
|
|
When sending attachments you read the entire file to calculate the file size. This seems highly inefficient. There are, of course, various operating system specific ways to get this information, but even with straight C++ you can do the following:
fseek(hFile, 0, SEEK_END);
TotalSize = ftell(hFile);
fseek(hFile, 0, SEEK_SET);
The only problem with this is if the total size of the file is greater than 4GB (clearly too big for an attachment, but you know those crazy users...). With VS, at least, you can use _ftelli64, but I don't know if there's a cross platform equivalent. You're current code also will fail with excessively sized files. Admittedly though, this is unexpected input.
|
|
|
|
|
Sorry, the second line should be:
FileSize = ftell(hFile);
TotalSize += FileSize;
or simply:
TotalSize += ftell(hFile);
|
|
|
|
|
GKarRacer,
Great point. Also, in the process of putting this in, I realized a couple of things that aren't good about rev 2.2. Now it reads the attachments one time just to check for total size and throws an error there if it is too large. However, in doing so:
1. Like you mentioned, it reads the whole file in the process just to find out how large it is.
2. It doesn't close the files so they will be left locked.
3. It still throws an error in the middle of sending so that if the file size changes between when it checks the first time and when it sends them, it could still end up in limbo.
To fix these problems I've made the following changes.
Starting on line 533:
TotalSize = 0;
for(FileId=0;FileId<Attachments.size();FileId++)
{
snprintf(FileName, 255, Attachments[FileId].c_str());
hFile = fopen(FileName,"rb");
if(hFile == NULL)
throw ECSmtp(ECSmtp::FILE_NOT_EXIST);
fseek(hFile, 0, SEEK_END);
FileSize = ftell(hFile);
TotalSize += FileSize;
if(TotalSize/1024 > MSG_SIZE_IN_MB*1024)
throw ECSmtp(ECSmtp::MSG_TOO_BIG);
fclose(hFile);
}
and then after making that change, starting on line 647:
fseek(hFile, 0, SEEK_END);
FileSize = ftell(hFile);
fseek (hFile,0,SEEK_SET);
MsgPart = 0;
for(i=0;i<FileSize/54+1;i++)
{
res = fread(FileBuf,sizeof(char),54,hFile);
MsgPart ? strcat(SendBuf,base64_encode(reinterpret_cast<const unsigned char*>(FileBuf),res).c_str())
: strcpy(SendBuf,base64_encode(reinterpret_cast<const unsigned char*>(FileBuf),res).c_str());
strcat(SendBuf,"\r\n");
MsgPart += res + 2;
if(MsgPart >= BUFFER_SIZE/2)
{ MsgPart = 0;
SendData(pEntry); }
}
if(MsgPart)
{
SendData(pEntry); }
fclose(hFile);
|
|
|
|
|